Bug 99154 – Refactor MacroAssembler interfaces to differentiate the pointer operands from the 64-bit integer operands

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

[Yuqiang Xian]

In current JavaScriptCore implementation for JSVALUE64 platform (i.e., the X64 platform), we assume that the JSValue size is same to the pointer size, and thus EncodedJSValue is simply type defined as a "void*". In the JIT compiler, we also take this assumption and invoke the same macro assembler interfaces for both JSValue and pointer operands. We need to differentiate the operations on pointers from the operations on JSValues, and let them invoking different macro assembler interfaces. For example, we now use the interface of "loadPtr" to load either a pointer or a JSValue, and we need to switch to using "loadPtr" to load a pointer and some new "load64" interface to load a JSValue. This would help us supporting other JSVALUE64 platforms where pointer size is not necessarily 64-bits, for example x32 (bug #99153).

The major modification I made is to introduce the "*64" interfaces in the MacroAssembler for those operations on JSValues, keep the "*Ptr" interfaces for those operations on real pointers, and go through all the JIT compiler code to correct the usage.

Patch forthcoming.

[Yuqiang Xian]

Created attachment 168383 [details]
patch

Not marking r? yet. Can you please let me know if the approach is sound enough?

Thanks.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:360:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:371:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 33 files


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

[Gavin Barraclough]

Comment on attachment 168383 [details]
patch

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

(r-, comments to follow)

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:278
> +        explicit TrustedImm64(const void* value)

Do we really need this constructor? – might be nice to try to remove it – if you have a void*, you should be creating a TrustedImmPtr.

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:286
> +

Extra newline.

> Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:299
> +

Extra newline.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:1333
> +    move(TrustedImm64(reinterpret_cast<uintptr_t>(globalThis)), regT0);

I think this code is implicitly making an assumption about JSValue boxing.  I think this would be better:
    move(TrustedImm64(JSValue::encode(JSValue(globalThis))), regT0);

[Gavin Barraclough]

Hey Yuqiang,

This all looks good in principal – but:

(1) Please put up just a patch with MacroAssembler changes alone.  We should be able to refactor the MacroAssembler to support Imm64 & more *64* operations separately, and then land changes to the JIT to use it.  The runtime changes necessary for x32 should be separate again.

(2) The X86_64 MacroAssembler changes duplicate a lot of functionality, in copying & pasting many operations into Ptr & 64 variants.  E.g. xor64, xorPtr.  Instead I think we should try to constrain the concept of Ptr sized operations to the MacroAssembler, so on X86_64 <foo>Ptr should be remapped to <foo>64, as we currently map <foo>Ptr to <foo>32 on other platforms.  Going forwards we don't want to be copying & pasting all 64-bit operations to both Ptr & 64 variants in the X86_64 macro assembler.

[Yuqiang Xian]

Created attachment 168533 [details]
Split patch - part 1: only add the *64 interfaces to MacroAssembler

Gavin,

Thanks a lot for the comments.

I've split the original patch and addressed your comments. This is the part one for only adding the *64 interfaces to the MacroAssembler. Can you please take another look?

Thanks.

[Yuqiang Xian]

Created attachment 168570 [details]
Rebased: Split patch - part 1: only add the *64 interfaces to MacroAssembler

Re-base the patch after r131251.

[Gavin Barraclough]

Comment on attachment 168570 [details]
Rebased: Split patch - part 1: only add the *64 interfaces to MacroAssembler

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

Hi Yuquiang – this basically looks good, but per my previous comments please move the *Ptr->*64 wrapper functions from MacroAssemblerX86_64.h to MacroAssembler.h
That way, the Ptr->[32/64] mapping will be done in the same place for all platforms.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:862
> +    DataLabelPtr moveWithPatch(TrustedImm64 initialValue, RegisterID dest)

Hmmm, I need to think a little more about this – I think I'd be happy to land as is & fix later as necessary.
The mismatch between TrustedImm64/DataLabelPtr means I think is probably isn't correct.
We should probably either add DataLabel64, make DataLabelPtr map onto DataLabel32 or DataLabel64 as appropriate, or keep the argument to this function as TrustedImmPtr.
But we can probably land as is & revisit.

[Yuqiang Xian]

Gavin, Thanks for the comments.

(In reply to comment #7)
> (From update of attachment 168570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168570&action=review
> 
> Hi Yuquiang – this basically looks good, but per my previous comments please move the *Ptr->*64 wrapper functions from MacroAssemblerX86_64.h to MacroAssembler.h
> That way, the Ptr->[32/64] mapping will be done in the same place for all platforms.

Okay. Will do it.

> 
> > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:862
> > +    DataLabelPtr moveWithPatch(TrustedImm64 initialValue, RegisterID dest)
> 
> Hmmm, I need to think a little more about this – I think I'd be happy to land as is & fix later as necessary.
> The mismatch between TrustedImm64/DataLabelPtr means I think is probably isn't correct.
> We should probably either add DataLabel64, make DataLabelPtr map onto DataLabel32 or DataLabel64 as appropriate, or keep the argument to this function as TrustedImmPtr.
> But we can probably land as is & revisit.

Thanks for pointing it out. Yes I think we need to fix it in this patch. It happens to not cause any problem as there's no use for it (even in my x32 code).

[Yuqiang Xian]

Created attachment 168840 [details]
Updated: Split patch - part1: add the*64 interfaces to MacroAssemblerX86_64

Patch updated addressing Gavin's comments.

Gavin, could you please help review it again?  Thanks for the efforts.

[Yuqiang Xian]

(In reply to comment #8)
> Gavin, Thanks for the comments.
> 
> (In reply to comment #7)
> > (From update of attachment 168570 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=168570&action=review
> > 
> > > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:862
> > > +    DataLabelPtr moveWithPatch(TrustedImm64 initialValue, RegisterID dest)
> > 
> > Hmmm, I need to think a little more about this – I think I'd be happy to land as is & fix later as necessary.
> > The mismatch between TrustedImm64/DataLabelPtr means I think is probably isn't correct.
> > We should probably either add DataLabel64, make DataLabelPtr map onto DataLabel32 or DataLabel64 as appropriate, or keep the argument to this function as TrustedImmPtr.
> > But we can probably land as is & revisit.
> 
> Thanks for pointing it out. Yes I think we need to fix it in this patch. It happens to not cause any problem as there's no use for it (even in my x32 code).

I just removed those methods in the new patch, since they're not used now (and won't be used in the near future).

[Yuqiang Xian]

Created attachment 168845 [details]
Updated: Split patch - part1: add the*64 interfaces to MacroAssemblerX86_64

more clean-up.

[Gavin Barraclough]

Comment on attachment 168845 [details]
Updated: Split patch - part1: add the*64 interfaces to MacroAssemblerX86_64

Looks great Yuqiang, thanks for the cleanup.  I'm out tomorrow, so you may want to cc Filip on your patches, or I'll be back Weds.

[Yuqiang Xian]

Part 1 landed as http://trac.webkit.org/changeset/131426.

Thanks for the review, Gavin.

[Yuqiang Xian]

Created attachment 168951 [details]
Split patch - part 2: let the JIT compilers use the *64 interfaces of MacroAssemblerX86_64

Patch part 2 - having the JIT compilers use the *64 interfaces for 64-bit integers, especially, the encoded JSValues which are now defined as int64_t.

Gavin/Filip, would you please take a look? Thanks.

[Yuqiang Xian]

Created attachment 169076 [details]
Rebased: Split patch - part 2: let the JIT compilers use the *64 interfaces of MacroAssemblerX86_64

Re-base the patch part2 after r131516.

[Yuqiang Xian]

Created attachment 169306 [details]
Another rebase: Split patch - part 2: let the JIT compilers use the *64 interfaces of MacroAssemblerX86_64

[Yuqiang Xian]

well... r131645 is rolled out again - so it means we need yet another rebase...

Gavin, what do you think? Should we wait for that patch to be fixed and merged again and then we can start this patch review, or should we go first?

Thanks.

[Gavin Barraclough]

Comment on attachment 169306 [details]
Another rebase: Split patch - part 2: let the JIT compilers use the *64 interfaces of MacroAssemblerX86_64

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

Hi Yuquiang, this basically all looks good to me.  I've really just picked up on one issue throughout, which is that the #if CPU(X86_64) all needs to be USE(JSVALUE64).
However I would like you to take the runtime changes out of this patch – this is a big refactoring, and breaking up into separate patches helps track down regression.  Please put up a new patch with just the jit & assembler changes, (& the review comments fixed), and then put up the runtime changes separately.  Sorry that this is a pain, but it is important if there is a problem later.

> Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:102
> +#if CPU(X86_64)

This is a bit unfortunate – when registers where pointer sized 'storePtr' meant "store this entire register".  Oh well.
I think this may be slightly better as:
#if USE(JSVALUE64)
And to change the storePtr in the else case to store32.

> Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:180
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & store32.

> Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:218
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & load32.

> Source/JavaScriptCore/dfg/DFGScratchRegisterAllocator.h:131
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & store32.

> Source/JavaScriptCore/dfg/DFGScratchRegisterAllocator.h:181
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & load32.

> Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h:64
>      LoadPtr,

Do we still need LoadPtr? – I'm gussing that on JSVALUE64 this is equivalent to Load64, and on JSVALUE32_64 this is equivalent to Load32Payload.  Ditto for StorePtr above.  It could be good to remove these enum cases – and the associated code to handle them, and just have a #define in this header mapping LoadPtr/StorePtr appropriately.

> Source/JavaScriptCore/dfg/DFGThunks.cpp:48
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & store32.

> Source/JavaScriptCore/dfg/DFGThunks.cpp:80
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & load32.

> Source/JavaScriptCore/dfg/DFGThunks.cpp:128
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - #if USE(JSVALUE64).

> Source/JavaScriptCore/dfg/DFGThunks.cpp:155
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & store32.

> Source/JavaScriptCore/jit/JITStubCall.h:140
> +#if CPU(X86_64)

See comment in emitPutToCallFrameHeader - #if USE(JSVALUE64)

[Yuqiang Xian]

Gavin, Thanks for your comments.

(In reply to comment #18)
> (From update of attachment 169306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169306&action=review
> 
> Hi Yuquiang, this basically all looks good to me.  I've really just picked up on one issue throughout, which is that the #if CPU(X86_64) all needs to be USE(JSVALUE64).

OK. Will do it.

> However I would like you to take the runtime changes out of this patch – this is a big refactoring, and breaking up into separate patches helps track down regression.  Please put up a new patch with just the jit & assembler changes, (& the review comments fixed), and then put up the runtime changes separately.  Sorry that this is a pain, but it is important if there is a problem later.

I'm not sure if it's doable. The only runtime changes I made is the definition of EncodedJSValue which is changed from "void*" to "int64_t", and the subsequent changes in the related JSValue methods. This is necessary as we now use the Imm64 interfaces in the JIT compilers to deal with a JSValue constant. The Imm64 interfaces only accept a int64_t data (and that's the intention). So I don't think it should be separated from the JIT compiler changes. What do you think, any suggestions? Thanks.

> 
> > Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:102
> > +#if CPU(X86_64)
> 
> This is a bit unfortunate – when registers where pointer sized 'storePtr' meant "store this entire register".  Oh well.
> I think this may be slightly better as:
> #if USE(JSVALUE64)
> And to change the storePtr in the else case to store32.
> 
> > Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:180
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & store32.
> 
> > Source/JavaScriptCore/dfg/DFGAssemblyHelpers.h:218
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & load32.
> 
> > Source/JavaScriptCore/dfg/DFGScratchRegisterAllocator.h:131
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & store32.
> 
> > Source/JavaScriptCore/dfg/DFGScratchRegisterAllocator.h:181
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & load32.
> 
> > Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h:64
> >      LoadPtr,
> 
> Do we still need LoadPtr? – I'm gussing that on JSVALUE64 this is equivalent to Load64, and on JSVALUE32_64 this is equivalent to Load32Payload.  Ditto for StorePtr above.  It could be good to remove these enum cases – and the associated code to handle them, and just have a #define in this header mapping LoadPtr/StorePtr appropriately.
> 

I think we still need the LoadPtr and StorePtr. Because we have the cases to load/store an unboxed JSCell and a Storage pointer, which are operations on the raw pointers.

> > Source/JavaScriptCore/dfg/DFGThunks.cpp:48
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & store32.
> 
> > Source/JavaScriptCore/dfg/DFGThunks.cpp:80
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & load32.
> 
> > Source/JavaScriptCore/dfg/DFGThunks.cpp:128
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - #if USE(JSVALUE64).
> 
> > Source/JavaScriptCore/dfg/DFGThunks.cpp:155
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - USE(JSVALUE64) & store32.
> 
> > Source/JavaScriptCore/jit/JITStubCall.h:140
> > +#if CPU(X86_64)
> 
> See comment in emitPutToCallFrameHeader - #if USE(JSVALUE64)

[Yuqiang Xian]

Created attachment 169543 [details]
Updated: Split patch - part 2: let the JIT compilers use the *64 interfaces of MacroAssemblerX86_64

Patch addressing Gavin's comments on changing the "if CPU(X86_64)"s to "if USE(JSVALUE64)".

[Gavin Barraclough]

Comment on attachment 169543 [details]
Updated: Split patch - part 2: let the JIT compilers use the *64 interfaces of MacroAssemblerX86_64

Looks good to me.

[Yuqiang Xian]

Patch part2 landed as <http://trac.webkit.org/changeset/131858>.

Marking this bug as fixed as I think all the stuffs for the MacroAssembler refactoring and JIT compilers usage for the new interfaces are committed.

We still need some more work for real x32 support of the JavaScriptCore JIT compilers (but now becomes really easier), which will be tracked in bug #99153.
Also there we may want the similar work on the LLInt (the offline assembler and the LLInt implementation).

Thanks.

[Roger Fong]

This patch has caused a regression here:
https://bugs.webkit.org/show_bug.cgi?id=100789