Hi All, I will submit a patch to re-enable JIT_OPTIMIZE_NATIVE_CALL on MIPS. One issue that I have is in JITOpcodes.cpp. Ex: JIT::Label JIT::privateCompileCTINativeCall(JSGlobalData* globalData, bool isConstruct) { ... poke(callFrameRegister, 1 + OBJECT_OFFSETOF(struct JITStackFrame, callFrame) / sizeof (void*)); <------- poke(ImmPtr(FunctionPtr(ctiVMThrowTrampoline).value())); ret(); return nativeCallThunk; } Why do we put callFrameRegister to the stack location of 1 + OBJECT_OFFSETOF(struct JITStackFrame, callFrame) / sizeof (void*)? Previously, we put it to the location of OBJECT_OFFSETOF(struct JITStackFrame, callFrame) / sizeof (void*). Thanks! Regards, Chao-ying
Created attachment 57903 [details] MIPS Native Call Support Here is the patch. Thanks! 0 regressions found. 0 tests fixed. OK.
Created attachment 57912 [details] Update Platform.h Resolve conflicts in Platform.h . Thanks!
Attachment 57912 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/jit/JITOpcodes.cpp:273: Extra space before ( in function call [whitespace/parens] [4] JavaScriptCore/wtf/Platform.h:973: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 57924 [details] Returned value in a register Ok. The return value is now in a register. Need to fix the MIPS support. Thanks!
Attachment 57924 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 JavaScriptCore/wtf/Platform.h:973: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Could someone review this patch and help to answer the question in my first post? Thanks a lot! Regards, Chao-ying
> Why do we put callFrameRegister to the stack location of 1 + OBJECT_OFFSETOF(struct JITStackFrame, callFrame) / sizeof (void*)? We need "1 +" because the return address has been pushed onto the stack by call. In the old implementation, we would pop the return address off the stack before this code ran.
(In reply to comment #7) > > Why do we put callFrameRegister to the stack location of 1 + OBJECT_OFFSETOF(struct JITStackFrame, callFrame) / sizeof (void*)? > > We need "1 +" because the return address has been pushed onto the stack by call. > > In the old implementation, we would pop the return address off the stack before this code ran. Thanks for the explanation! This helps me to understand. For MIPS native call, I manage the return address in the stack differently (at the -4 location). I use "#if CPU(MIPS)" in several places to make MIPS work. Thanks! Regards, Chao-ying
Comment on attachment 57924 [details] Returned value in a register Sorry for the delay in review > +#if CPU(MIPS) > + // move the returned address to regT1 > + move(MIPSRegisters::ra, regT1); > +#else > peek(regT1); > +#endif This is bad, we should simply have a loadReturnAddress(RegisterID dst) function to do this, rather than having an architecture check everywhere we need to do this (eg. ARM, PPC, etc would need to have a branch as well) > emitPutToCallFrameHeader(regT1, RegisterFile::ReturnPC); > > #if CPU(X86_64) > @@ -204,6 +209,31 @@ JIT::Label JIT::privateCompileCTINativeC > > addPtr(Imm32(16 - sizeof(void*)), stackPointerRegister); > > +#elif CPU(MIPS) > + // Calling convention: f(a0, a1, a2, a3); > + // Host function signature: f(ExecState*); > + > + // Allocate stack space for 24 bytes (8-byte aligned) > + // 16 bytes (unused) for 4 arguments > + // 4 bytes (unused) > + // 4 bytes for the returned address > + subPtr(Imm32(24), stackPointerRegister); > + > + // Save the returned address to 20($sp) > + storePtr(MIPSRegisters::ra, Address(stackPointerRegister, 20)); > + > + // Setup arg0 > + move(callFrameRegister, MIPSRegisters::a0); > + > + // Call > + emitGetFromCallFrameHeaderPtr(RegisterFile::Callee, MIPSRegisters::a2); > + loadPtr(Address(MIPSRegisters::a2, OBJECT_OFFSETOF(JSFunction, m_executable)), regT2); > + move(regT0, callFrameRegister); // Eagerly restore caller frame register to avoid loading from stack. > + call(Address(regT2, executableOffsetToFunction)); > + > + // Restore stack space > + addPtr(Imm32(24), stackPointerRegister); > + Our normal model for a callframe this complicated is to make a struct that describes it and then use OBJECT_OFFSETOF to do the stores > +#if CPU(MIPS) > + loadPtr(Address(stackPointerRegister, -4), MIPSRegisters::ra); > +#endif -4? should be -sizeof(void*)? > + > // Return. > ret(); > > // Handle an exception > exceptionHandler.link(this); > // Grab the return address. > +#if CPU(MIPS) > + loadPtr(Address(stackPointerRegister, -4), regT1); > +#else > peek(regT1); > +#endif why doesn't peek() work here? > +#if CPU(MIPS) > + poke(callFrameRegister, OBJECT_OFFSETOF(struct JITStackFrame, callFrame) / sizeof(void*)); > + move(ImmPtr(FunctionPtr(ctiVMThrowTrampoline).value()), MIPSRegisters::ra); > +#else > poke(callFrameRegister, 1 + OBJECT_OFFSETOF(struct JITStackFrame, callFrame) / sizeof (void*)); > poke(ImmPtr(FunctionPtr(ctiVMThrowTrampoline).value())); > +#endif a setReturnAddress function is probably better here
Created attachment 59421 [details] Revise the patch for all CPUs Hi Oliver, I revised the patch, so that all CPUs can share the same mechanism (based on the original JITOpcodes.cpp). The patch uses the standard functions to access and restore the returned address. The callFrameRegister is restored after the native call. People may need to test x86. Thanks for your review! Regards, Chao-ying
Comment on attachment 59421 [details] Revise the patch for all CPUs Need to merge the patch. Will submit it soon.
Created attachment 59534 [details] A new version for MIPS Native Call Here is a new version that follows the current design. Thanks! Regards, Chao-ying
Created attachment 59544 [details] Use svn-create-patch I forgot to use svn-create-patch for the previous one. Sent it again. Thanks!
> + || (CPU(MIPS) && PLATFORM(QT) && OS(LINUX)) It should *not* be platform dependent. Probably you could also remove the OS check. I do not hear any attempt to port MIPS-JIT on other OS than linux. Am I right?
(In reply to comment #14) > > + || (CPU(MIPS) && PLATFORM(QT) && OS(LINUX)) > > It should *not* be platform dependent. Ok. > > Probably you could also remove the OS check. I do not hear any attempt to port MIPS-JIT on other OS than linux. Am I right? I guess so. I don't hear, either. Thanks! Regards, Chao-ying
Created attachment 59675 [details] Update Platform.h Platform.h just checks CPU(MIPS). Thanks!
Comment on attachment 59675 [details] Update Platform.h Clearing flags on attachment: 59675 Committed r62422: <http://trac.webkit.org/changeset/62422>
All reviewed patches have been landed. Closing bug.