Bug 40179 – Re-enable JIT_OPTIMIZE_NATIVE_CALL on MIPS

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Chao-ying Fu]

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

[Chao-ying Fu]

Created attachment 57903 [details]
MIPS Native Call Support

Here is the patch.  Thanks!

0 regressions found.
0 tests fixed.
OK.

[Chao-ying Fu]

Created attachment 57912 [details]
Update Platform.h

Resolve conflicts in Platform.h . Thanks!

[WebKit Review Bot]

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.

[Chao-ying Fu]

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!

[WebKit Review Bot]

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.

[Chao-ying Fu]

Could someone review this patch and help to answer the question in my first post?  Thanks a lot!

Regards,
Chao-ying

[Geoffrey Garen]

> 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.

[Chao-ying Fu]

(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

[Oliver Hunt]

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

[Chao-ying Fu]

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

[Chao-ying Fu]

Comment on attachment 59421 [details]
Revise the patch for all CPUs

Need to merge the patch.  Will submit it soon.

[Chao-ying Fu]

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

[Chao-ying Fu]

Created attachment 59544 [details]
Use svn-create-patch

I forgot to use svn-create-patch for the previous one.  Sent it again.  Thanks!

[Gabor Loki]

> +     || (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?

[Chao-ying Fu]

(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

[Chao-ying Fu]

Created attachment 59675 [details]
Update Platform.h

Platform.h just checks CPU(MIPS).  Thanks!

[WebKit Commit Bot]

Comment on attachment 59675 [details]
Update Platform.h

Clearing flags on attachment: 59675

Committed r62422: <http://trac.webkit.org/changeset/62422>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.