Bug 84449 – DFG JIT is not ARM EABI compatible

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

[Yong Li]

EABI requies that 64-bit integer when being passed as argument must start at an even number register (r0-r1 or r2-r3). But current DFG JIT doesn't take care of this, and assumes 64-bit integer can be passed through r1, r2.

I have a patch to work around this issue (will submit it soon). But that looks ugly. Any idea about a neat solution?

Also I don't see DFG boosts performance on sunspider. Actually DFG gives worse result. Any idea about this?

[Yong Li]

Created attachment 138093 [details]
the ugly work around

EncodedJSValue is 64-bit integer for JSVALUE32_64. When being passed as argument, an EncodedJSValue is aligned to r0 or r2.

iOS may have its own EABI...

One solution may be splitting EncodedJSValue arguments into 32-bit integers for those DFG operation calls.

[Gavin Barraclough]

Interesting, looking at the iOS developer docs, this may be a difference on iOS:

http://developer.apple.com/library/ios/#documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html#//apple_ref/doc/uid/TP40009021-SW1

    * Large data types (larger than 4 bytes) are 4-byte aligned.

We don't want to have to duplicate all these method to have two signatures, I guess we could provide a set of wrappers on non-iOS ARM, but the cleanest approach may just be to fix the callOperation interface to align the arguments correctly.  If this degrades performance (by moving augments from registers to the stack) we can always change the order of args to functions affected to compensate.

[Yong Li]

(In reply to comment #2)
> Interesting, looking at the iOS developer docs, this may be a difference on iOS:
> 
> http://developer.apple.com/library/ios/#documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html#//apple_ref/doc/uid/TP40009021-SW1
> 
>     * Large data types (larger than 4 bytes) are 4-byte aligned.
> 
> We don't want to have to duplicate all these method to have two signatures, I guess we could provide a set of wrappers on non-iOS ARM, but the cleanest approach may just be to fix the callOperation interface to align the arguments correctly.  If this degrades performance (by moving augments from registers to the stack) we can always change the order of args to functions affected to compensate.

So how about the patch?

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > Interesting, looking at the iOS developer docs, this may be a difference on iOS:
> > 
> > http://developer.apple.com/library/ios/#documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARMv6FunctionCallingConventions.html#//apple_ref/doc/uid/TP40009021-SW1
> > 
> >     * Large data types (larger than 4 bytes) are 4-byte aligned.
> > 
> > We don't want to have to duplicate all these method to have two signatures, I guess we could provide a set of wrappers on non-iOS ARM, but the cleanest approach may just be to fix the callOperation interface to align the arguments correctly.  If this degrades performance (by moving augments from registers to the stack) we can always change the order of args to functions affected to compensate.
> 
> So how about the patch?

I like the general approach of your patch.  Let me know when it's ready to be reviewed.

[Yong Li]

(In reply to comment #4)
> (In reply to comment #3)
> > 
> > So how about the patch?
> 
> I like the general approach of your patch.  Let me know when it's ready to be reviewed.

Thanks. Sure I'll upload a ready-for-review patch.

[Yong Li]

Created attachment 139506 [details]
the patch

[Yong Li]

Comment on attachment 139506 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139506&action=review

> Source/JavaScriptCore/dfg/DFGOperations.cpp:102
> +// EncodedJSValue in JSVALUE32_64 is a 64-bit integer. When being compiled in ARM EABI, it must be aligned even-numbered register (r0, r2 or [sp]).

Hm... should be "aligned to"

[Filip Pizlo]

Comment on attachment 139506 [details]
the patch

R=me.  Sorry it took so long to review.

[Filip Pizlo]

*** Bug 86335 has been marked as a duplicate of this bug. ***

[Yong Li]

Comment on attachment 139506 [details]
the patch

Thanks!

[WebKit Review Bot]

Comment on attachment 139506 [details]
the patch

Clearing flags on attachment: 139506

Committed r116951: <http://trac.webkit.org/changeset/116951>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.