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.
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.
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?
(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?)
(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? :-)
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.
Created attachment 170544 [details] Patch - part1: add the <foo>q instructions support to the offline assembler Addressing Filip's comments. Thanks for the review.
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.
(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?
(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?
(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.
(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.
(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.
(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.
Part 1 landed as <http://trac.webkit.org/changeset/132445>. Thanks for the review.
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.
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.
Filip, what do you think? Thanks for your efforts.
(In reply to comment #17) > Filip, what do you think? > Thanks for your efforts. Busy now, will look later.
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.
Part2 landed as <https://trac.webkit.org/changeset/133551>. Thanks for the review. Closing this bug.