Bug 100321 – Refactor LLInt64 to distinguish the pointer operations from the 64-bit integer operations

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yuqiang Xian]

We have refactored the MacroAssembler and JIT compilers to distinguish the pointer operations from the 64-bit integer operations (see bug #99154).
Now we want to do the similar work for LLInt, and the goal is same as the one mentioned in 99154.

[Yuqiang Xian]

Created attachment 170542 [details]
Patch - part1: add the <foo>q instructions support to the offline assembler

This is the first part of the modification:
in the offline assembler, adding the support of the "<foo>q" instructions which will be used for 64-bit integer operations.

[Filip Pizlo]

Comment on attachment 170542 [details]
Patch - part1: add the <foo>q instructions support to the offline assembler

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

I think you should fix the X86_INSTRUCTIONS part but otherwise I like where this is going.

> Source/JavaScriptCore/offlineasm/instructions.rb:216
> +X86_64_INSTRUCTIONS =

The X86_INSTRUCTIONS, and ARMv7_INSTRUCTIONS are used for instructions that are truly artifacts of that architecture.

The cloop might be built in JSVALUE64 mode on non-X86 64-bit architectures.  Like, I dunno, PowerPC maybe.  I don't know if anyone does that, but it would be really weird to name these X86_64_INSTRUCTIONS and then have a PowerPC 64-bit port that uses them.

I suggest putting these instructions into the main instruction list.

> Source/JavaScriptCore/offlineasm/x86.rb:55
> -        "*#{x86Operand(kind)}"
> +        "*#{x86Operand(:quad)}"

Is that right?  Shouldn't this be :ptr?

[Yuqiang Xian]

(In reply to comment #2)
> 
> I think you should fix the X86_INSTRUCTIONS part but otherwise I like where this is going.
> 
> > Source/JavaScriptCore/offlineasm/instructions.rb:216
> > +X86_64_INSTRUCTIONS =
> 
> The X86_INSTRUCTIONS, and ARMv7_INSTRUCTIONS are used for instructions that are truly artifacts of that architecture.
> 
> The cloop might be built in JSVALUE64 mode on non-X86 64-bit architectures.  Like, I dunno, PowerPC maybe.  I don't know if anyone does that, but it would be really weird to name these X86_64_INSTRUCTIONS and then have a PowerPC 64-bit port that uses them.
> 
Thanks for the comments. Will fix it.

> I suggest putting these instructions into the main instruction list.
> 
> > Source/JavaScriptCore/offlineasm/x86.rb:55
> > -        "*#{x86Operand(kind)}"
> > +        "*#{x86Operand(:quad)}"
> 
> Is that right?  Shouldn't this be :ptr?

It won't cause errors as this code only runs in X64 backend. I would say that this is a necessary change for x32 support. (partial registers are not allowed to be call/jump operands on x32. Maybe I can revert this change and put it in later x32 specific patch, or keep it here as it doesn't do harm, what do you think?)

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > 
> > I think you should fix the X86_INSTRUCTIONS part but otherwise I like where this is going.
> > 
> > > Source/JavaScriptCore/offlineasm/instructions.rb:216
> > > +X86_64_INSTRUCTIONS =
> > 
> > The X86_INSTRUCTIONS, and ARMv7_INSTRUCTIONS are used for instructions that are truly artifacts of that architecture.
> > 
> > The cloop might be built in JSVALUE64 mode on non-X86 64-bit architectures.  Like, I dunno, PowerPC maybe.  I don't know if anyone does that, but it would be really weird to name these X86_64_INSTRUCTIONS and then have a PowerPC 64-bit port that uses them.
> > 
> Thanks for the comments. Will fix it.
> 
> > I suggest putting these instructions into the main instruction list.
> > 
> > > Source/JavaScriptCore/offlineasm/x86.rb:55
> > > -        "*#{x86Operand(kind)}"
> > > +        "*#{x86Operand(:quad)}"
> > 
> > Is that right?  Shouldn't this be :ptr?
> 
> It won't cause errors as this code only runs in X64 backend. I would say that this is a necessary change for x32 support. (partial registers are not allowed to be call/jump operands on x32. Maybe I can revert this change and put it in later x32 specific patch, or keep it here as it doesn't do harm, what do you think?)

Ah - right!  That makes sense.  No, don't worry about it.  I think you can keep this.  But maybe you can put a comment that explains what you just said? :-)

[Filip Pizlo]

Comment on attachment 170542 [details]
Patch - part1: add the <foo>q instructions support to the offline assembler

Resetting to r?  I want Mark to look at the cloop changes.  r=me on the x86 changes.

[Yuqiang Xian]

Created attachment 170544 [details]
Patch - part1: add the <foo>q instructions support to the offline assembler

Addressing Filip's comments. Thanks for the review.

[Mark Lam]

Comment on attachment 170544 [details]
Patch - part1: add the <foo>q instructions support to the offline assembler

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

cloop changes look good to me.  Not so sure about some of the x86 changes though.

> Source/JavaScriptCore/offlineasm/x86.rb:56
> +        "*#{x86Operand(:quad)}"

What happens when we're building for 32-bit x86?  Shouldn't this be :ptr instead of :quad?

> Source/JavaScriptCore/offlineasm/x86.rb:251
> +        isX64 ? "*#{x86Operand(:quad)}" : "*#{x86Operand(:ptr)}"

Does isX64 necessarily mean we'll be using 64bit pointers?  Otherwise, it seems that this should always :ptr instead of :quad.

[Yuqiang Xian]

(In reply to comment #7)
> (From update of attachment 170544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170544&action=review
> 
> cloop changes look good to me.  Not so sure about some of the x86 changes though.
> 
> > Source/JavaScriptCore/offlineasm/x86.rb:56
> > +        "*#{x86Operand(:quad)}"
> 
> What happens when we're building for 32-bit x86?  Shouldn't this be :ptr instead of :quad?
> 
> > Source/JavaScriptCore/offlineasm/x86.rb:251
> > +        isX64 ? "*#{x86Operand(:quad)}" : "*#{x86Operand(:ptr)}"
> 
> Does isX64 necessarily mean we'll be using 64bit pointers?  Otherwise, it seems that this should always :ptr instead of :quad.

Hi Mark, thanks for the review. For your questions on this couple of changes, I think comment #3 answers them?

[Mark Lam]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 170544 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170544&action=review
> > 
> > cloop changes look good to me.  Not so sure about some of the x86 changes though.
> > 
> > > Source/JavaScriptCore/offlineasm/x86.rb:56
> > > +        "*#{x86Operand(:quad)}"
> > 
> > What happens when we're building for 32-bit x86?  Shouldn't this be :ptr instead of :quad?
> > 
> > > Source/JavaScriptCore/offlineasm/x86.rb:251
> > > +        isX64 ? "*#{x86Operand(:quad)}" : "*#{x86Operand(:ptr)}"
> > 
> > Does isX64 necessarily mean we'll be using 64bit pointers?  Otherwise, it seems that this should always :ptr instead of :quad.
> 
> Hi Mark, thanks for the review. For your questions on this couple of changes, I think comment #3 answers them?

Based on this, the line at x86.rb:56 should be change to test for isX64 as well like at line 251.  Isn't that right?  For 32-bit x86 (not x32), I would expect jmps and calls to use a 32bit ptr, not a quad.  Am I misunderstanding something?

[Yuqiang Xian]

(In reply to comment #9)
> Based on this, the line at x86.rb:56 should be change to test for isX64 as well like at line 251.  Isn't that right?  For 32-bit x86 (not x32), I would expect jmps and calls to use a 32bit ptr, not a quad.  Am I misunderstanding something?

Hi Mark, line 56 will be run only in X64 backend. It's guarded by line 40. Thanks.

[Yuqiang Xian]

(In reply to comment #10)
> (In reply to comment #9)
> > Based on this, the line at x86.rb:56 should be change to test for isX64 as well like at line 251.  Isn't that right?  For 32-bit x86 (not x32), I would expect jmps and calls to use a 32bit ptr, not a quad.  Am I misunderstanding something?
> 
> Hi Mark, line 56 will be run only in X64 backend. It's guarded by line 40. Thanks.

Or, in other words, SpecialRegister should only be available in X64. Does this sound reasonable? Or we need to add another guard in this method? it's Ok for me to add the guard before landing if you think necessary. Thanks.

[Mark Lam]

(In reply to comment #10)
> (In reply to comment #9)
> > Based on this, the line at x86.rb:56 should be change to test for isX64 as well like at line 251.  Isn't that right?  For 32-bit x86 (not x32), I would expect jmps and calls to use a 32bit ptr, not a quad.  Am I misunderstanding something?
> 
> Hi Mark, line 56 will be run only in X64 backend. It's guarded by line 40. Thanks.

Ah, yes.  I missed that part.  It looks good to me then.  Thanks.

[Filip Pizlo]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Based on this, the line at x86.rb:56 should be change to test for isX64 as well like at line 251.  Isn't that right?  For 32-bit x86 (not x32), I would expect jmps and calls to use a 32bit ptr, not a quad.  Am I misunderstanding something?
> > 
> > Hi Mark, line 56 will be run only in X64 backend. It's guarded by line 40. Thanks.
> 
> Or, in other words, SpecialRegister should only be available in X64. Does this sound reasonable? Or we need to add another guard in this method? it's Ok for me to add the guard before landing if you think necessary. Thanks.

The reliance on SpecialRegister only existing in 64-bit is a little strange, but not strange enough for me to care.

R=me.

[Yuqiang Xian]

Part 1 landed as <http://trac.webkit.org/changeset/132445>.

Thanks for the review.

[Yuqiang Xian]

Created attachment 170553 [details]
Patch - part2: change the operations on 64-bit integers to use the "<foo>q" instructions in the LLInt

This is the second part of the modification: in the low level interpreter, changing the operations on 64-bit integers to use the "<foo>q" instructions.
This also removes some unused/meaningless "<foo>p" instructions.

[Mark Lam]

Comment on attachment 170553 [details]
Patch - part2: change the operations on 64-bit integers to use the "<foo>q" instructions in the LLInt

I didn't see any issues, but my eyes are a little tired.  Filip, I'm ok with it if you are.

[Yuqiang Xian]

Filip, what do you think?
Thanks for your efforts.

[Filip Pizlo]

(In reply to comment #17)
> Filip, what do you think?
> Thanks for your efforts.

Busy now, will look later.

[Yuqiang Xian]

Created attachment 172456 [details]
Rebased: Patch - part2: change the operations on 64-bit integers to use the "<foo>q" instructions in the LLInt

Rebased.

Could you please take a look? Thanks.

[Yuqiang Xian]

Part2 landed as <https://trac.webkit.org/changeset/133551>.

Thanks for the review. Closing this bug.