Bug 43999 - Support JSVALUE32_64 on MIPS
Summary: Support JSVALUE32_64 on MIPS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-13 18:22 PDT by Chao-ying Fu
Modified: 2010-09-06 13:29 PDT (History)
6 users (show)

See Also:


Attachments
JSVALUE32_64 on MIPS (29.43 KB, patch)
2010-08-13 18:30 PDT, Chao-ying Fu
no flags Details | Formatted Diff | Diff
New patch to fix format error in the patch (29.25 KB, patch)
2010-08-13 18:37 PDT, Chao-ying Fu
no flags Details | Formatted Diff | Diff
Update padding in JITStackFrame (29.23 KB, patch)
2010-08-18 16:37 PDT, Chao-ying Fu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chao-ying Fu 2010-08-13 18:22:41 PDT
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
Comment 1 Chao-ying Fu 2010-08-13 18:30:15 PDT
Created attachment 64395 [details]
JSVALUE32_64 on MIPS

Here is the patch.  Thanks!
Comment 2 Chao-ying Fu 2010-08-13 18:37:03 PDT
Created attachment 64396 [details]
New patch to fix format error in the patch
Comment 3 WebKit Review Bot 2010-08-13 18:38:54 PDT
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.
Comment 4 Gabor Loki 2010-08-15 23:55:00 PDT
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;
Comment 5 Chao-ying Fu 2010-08-16 11:00:32 PDT
(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
Comment 6 Chao-ying Fu 2010-08-18 16:37:24 PDT
Created attachment 64788 [details]
Update padding in JITStackFrame

Here is a new patch to update padding in JITStackFrame.  Thanks!
Comment 7 Oliver Hunt 2010-08-26 16:58:40 PDT
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?
Comment 8 Chao-ying Fu 2010-08-26 17:15:52 PDT
(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
Comment 9 Oliver Hunt 2010-09-06 11:18:24 PDT
Comment on attachment 64788 [details]
Update padding in JITStackFrame

r=me
Comment 10 WebKit Commit Bot 2010-09-06 13:29:26 PDT
Comment on attachment 64788 [details]
Update padding in JITStackFrame

Clearing flags on attachment: 64788

Committed r66846: <http://trac.webkit.org/changeset/66846>
Comment 11 WebKit Commit Bot 2010-09-06 13:29:31 PDT
All reviewed patches have been landed.  Closing bug.