Bug 43999 – Support JSVALUE32_64 on MIPS

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

[Chao-ying Fu]

Hi All,

  I added some missing assembly functions to support JSVALUE32_64 on MIPS.
SunSpider test on a MIPS board is as follows.

JSVALUE32:    11045.8ms +/- 0.3%
JSVALUE32_64:  7290.5ms +/- 0.3%

  Regression test is ok.  I will send a patch soon.  Thanks!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 64395 [details]
JSVALUE32_64 on MIPS

Here is the patch.  Thanks!

[Chao-ying Fu]

Created attachment 64396 [details]
New patch to fix format error in the patch

[WebKit Review Bot]

Attachment 64396 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/Platform.h:916:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabor Loki]

The patch looks good to me, but I have some questions.

>      struct JITStackFrame {
>          void* reserved; // Unused
> +#if USE(JSVALUE32_64)
> +        void* padding1; // Maintain 8-byte stack alignment.
> +#endif

You could switch the type of 'reserved' member to JITStubArg which is a 32 bits wide data if JSVALUE32 is enabled, and 64 bits wide otherwise. So the alignment of the 'args' should be fine in both cases.

>          JITStubArg args[6];
>  
> +#if USE(JSVALUE32_64)
> +        void* padding2; // Maintain 8-byte stack alignment.
> +#endif
> +

I do not get why it is needed. If 'args' member is aligned, the preservedGP is aligned too.

>          void* preservedGP; // store GP when using PIC code
>          void* preservedS0;

[Chao-ying Fu]

(In reply to comment #4)
> The patch looks good to me, but I have some questions.
> 
> >      struct JITStackFrame {
> >          void* reserved; // Unused
> > +#if USE(JSVALUE32_64)
> > +        void* padding1; // Maintain 8-byte stack alignment.
> > +#endif
> 
> You could switch the type of 'reserved' member to JITStubArg which is a 32 bits wide data if JSVALUE32 is enabled, and 64 bits wide otherwise. So the alignment of the 'args' should be fine in both cases.

  Yes, this is a good idea.  I can use JITStubArg to avoid padding1.

> 
> >          JITStubArg args[6];
> >  
> > +#if USE(JSVALUE32_64)
> > +        void* padding2; // Maintain 8-byte stack alignment.
> > +#endif
> > +
> 
> I do not get why it is needed. If 'args' member is aligned, the preservedGP is aligned too.

  Yes, preservedGP is already aligned.  But I need padding2, so that the overall stack length can be a multiple of 8 bytes.  MIPS O32 ABI requires this.

> 
> >          void* preservedGP; // store GP when using PIC code
> >          void* preservedS0;

  Thanks!

Regards,
Chao-ying

[Chao-ying Fu]

Created attachment 64788 [details]
Update padding in JITStackFrame

Here is a new patch to update padding in JITStackFrame.  Thanks!

[Oliver Hunt]

Comment on attachment 64788 [details]
Update padding in JITStackFrame


> Index: JavaScriptCore/jit/JITPropertyAccess32_64.cpp
> ===================================================================
> +#if CPU(MIPS)
> +    // For MIPS, we don't add sizeof(void*) to the stack offset.
> +    load32(Address(stackPointerRegister, OBJECT_OFFSETOF(JITStackFrame, args[2]) + OBJECT_OFFSETOF(JSValue, u.asBits.payload)), regT3);
> +    load32(Address(stackPointerRegister, OBJECT_OFFSETOF(JITStackFrame, args[2]) + OBJECT_OFFSETOF(JSValue, u.asBits.tag)), regT2);
> +#else
>      load32(Address(stackPointerRegister, OBJECT_OFFSETOF(JITStackFrame, args[2]) + sizeof(void*) + OBJECT_OFFSETOF(JSValue, u.asBits.payload)), regT3);
>      load32(Address(stackPointerRegister, OBJECT_OFFSETOF(JITStackFrame, args[2]) + sizeof(void*) + OBJECT_OFFSETOF(JSValue, u.asBits.tag)), regT2);
> +#endif
>      

I don't particularly like this change -- in general we don't want to add ifdefs unless absolutely necessary, i assume the void* is for the return address here which makes me wonder why MIPS needs it when ARM doesn't -- does arm put the return address on the stack?

[Chao-ying Fu]

(In reply to comment #7)
> (From update of attachment 64788 [details])
> 
> > Index: JavaScriptCore/jit/JITPropertyAccess32_64.cpp
> > ===================================================================
> > +#if CPU(MIPS)
> > +    // For MIPS, we don't add sizeof(void*) to the stack offset.
> > +    load32(Address(stackPointerRegister, OBJECT_OFFSETOF(JITStackFrame, args[2]) + OBJECT_OFFSETOF(JSValue, u.asBits.payload)), regT3);
> > +    load32(Address(stackPointerRegister, OBJECT_OFFSETOF(JITStackFrame, args[2]) + OBJECT_OFFSETOF(JSValue, u.asBits.tag)), regT2);
> > +#else
> >      load32(Address(stackPointerRegister, OBJECT_OFFSETOF(JITStackFrame, args[2]) + sizeof(void*) + OBJECT_OFFSETOF(JSValue, u.asBits.payload)), regT3);
> >      load32(Address(stackPointerRegister, OBJECT_OFFSETOF(JITStackFrame, args[2]) + sizeof(void*) + OBJECT_OFFSETOF(JSValue, u.asBits.tag)), regT2);
> > +#endif
> >      
> 
> I don't particularly like this change -- in general we don't want to add ifdefs unless absolutely necessary, i assume the void* is for the return address here which makes me wonder why MIPS needs it when ARM doesn't -- does arm put the return address on the stack?

  This stack offset issue took me several days to debug MIPS JIT code and finally found out that the stack offset was wrong on MIPS.  I don't know why ARM needs sizeof(void*), similar to X86.  Thanks!

Regards,
Chao-ying

[Oliver Hunt]

Comment on attachment 64788 [details]
Update padding in JITStackFrame

r=me

[WebKit Commit Bot]

Comment on attachment 64788 [details]
Update padding in JITStackFrame

Clearing flags on attachment: 64788

Committed r66846: <http://trac.webkit.org/changeset/66846>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.