WebKit does not build on MIPS due to the change in http://trac.webkit.org/changeset/200586 . A special case is needed for MIPS as well as x86 because MIPS doesn't have enough registers either, and the function call ABI is different on MIPS.
I am working on a patch.
Created attachment 279023 [details] Patch A patch addressing the issue
This is the code my patch generates: move a0,s8 move a1,zero sw v1,-12(s8) sw v0,-16(s8) lw t6,-20(s8) lw v0,-24(s8) sw t5,-36(s8) sw t4,-40(s8) lw v1,-12(s8) lw t4,-16(s8) move a2,v0 move a3,t6 sw t2,16(sp) sw t3,20(sp) sw t4,24(sp) sw v1,28(sp) sw t3,-28(s8) sw t2,-32(s8) lw v0,-36(s8) lw v1,-40(s8) sw v1,32(sp) sw v0,36(sp) li t0,2 sw t0,28(s8) lui t9,0x1d3 ori t9,t9,0x33f4 jalr t9 nop I am wondering whether there is a way to have basePayload and baseTag load directly into the right argument registers, which would save these two moves.
Comment on attachment 279023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279023&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2931 > + m_jit.move(TrustedImm32(0), GPRInfo::argumentGPR1); Is this because 64-bit values need to be aligned on even registers?
Comment on attachment 279023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279023&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2933 > + { If we won't find a way to avoid excessive moves, I propose to merge this code with X86 code path. Only top 4 lines are different for MIPS, everything else looks like an exact copy of X86
Comment on attachment 279023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279023&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2933 >> + { > > If we won't find a way to avoid excessive moves, I propose to merge this code with X86 code path. Only top 4 lines are different for MIPS, everything else looks like an exact copy of X86 IMO, this will make the code harder to read. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2947 > + m_jit.move(basePayload, GPRInfo::argumentGPR2); > + m_jit.move(baseTag, GPRInfo::argumentGPR3); This is wrong if basePayload/baseTag are aliased to argument registers w/ each other. i.e, if basePayload is argumentGPR3 and baseTag is argumentGPR2, this code will do the wrong thing.
Comment on attachment 279023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279023&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2931 >> + m_jit.move(TrustedImm32(0), GPRInfo::argumentGPR1); > > Is this because 64-bit values need to be aligned on even registers? That was my reasoning. Though looking at the code generated by gcc for operationPutByValWithThisStrict(), even in -O0 it never accesses $a1 or the corresponding address on the stack, so I will remove that one line in a subsequent version of the patch. The next value (which is 64 bits) definitely needs to be on $a2 and $a3 though (and accordingly, 4 stack entries need to be reserved for the values we put in $a0-$a3, though they don't need to be set by the caller. >>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2933 >>> + { >> >> If we won't find a way to avoid excessive moves, I propose to merge this code with X86 code path. Only top 4 lines are different for MIPS, everything else looks like an exact copy of X86 > > IMO, this will make the code harder to read. Guilty as charged: I copy-pasted the x86 code and worked from there, I think I was expecting more difference in the end. But yeah, I guess it's a matter of avoiding code duplication vs readability. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2947 >> + m_jit.move(baseTag, GPRInfo::argumentGPR3); > > This is wrong if basePayload/baseTag are aliased to argument registers w/ each other. > i.e, if basePayload is argumentGPR3 and baseTag is argumentGPR2, this code will do the wrong thing. I thought this was not possible because the argumentGPR's are not in GPRInfo::toRegister() (and not counted in GPRInfo::numberOfRegisters). Including them might be an idea for the future though, as I suspect more registers available might improve performances (and ARM does that so it's probably possible), but I think that's a trickier change, and I'd like to get the build unbroken.
Comment on attachment 279023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279023&action=review >>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2947 >>> + m_jit.move(baseTag, GPRInfo::argumentGPR3); >> >> This is wrong if basePayload/baseTag are aliased to argument registers w/ each other. >> i.e, if basePayload is argumentGPR3 and baseTag is argumentGPR2, this code will do the wrong thing. > > I thought this was not possible because the argumentGPR's are not in GPRInfo::toRegister() (and not counted in GPRInfo::numberOfRegisters). Including them might be an idea for the future though, as I suspect more registers available might improve performances (and ARM does that so it's probably possible), but I think that's a trickier change, and I'd like to get the build unbroken. I don't quite understand your response here. Tag/Payload regs will be allocated by the DFG's register allocator. It will happily use argument registers. I don't think this makes the code harder to read. All code we write that sets up a call frame must account for this.
Comment on attachment 279023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279023&action=review >>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2947 >>>> + m_jit.move(baseTag, GPRInfo::argumentGPR3); >>> >>> This is wrong if basePayload/baseTag are aliased to argument registers w/ each other. >>> i.e, if basePayload is argumentGPR3 and baseTag is argumentGPR2, this code will do the wrong thing. >> >> I thought this was not possible because the argumentGPR's are not in GPRInfo::toRegister() (and not counted in GPRInfo::numberOfRegisters). Including them might be an idea for the future though, as I suspect more registers available might improve performances (and ARM does that so it's probably possible), but I think that's a trickier change, and I'd like to get the build unbroken. > > I don't quite understand your response here. Tag/Payload regs will be allocated by the DFG's register allocator. It will happily use argument registers. > I don't think this makes the code harder to read. All code we write that sets up a call frame must account for this. Sorry if that wasn't clear, and there is a possibility that I misunderstand something. What I mean is that DFG::RegisterBank<GPRInfo>::allocate() (see ::allocateInternal()) uses GPRInfo::toRegister() (and GPRInfo::numberOfRegisters as RegisterBank::NUM_REGS) as its source of registers. In the case of MIPS, GPRInfo::toRegister() never returns an argumentGPRx. Therefore baseTag and basePayload cannot be an argumentGPRx register. That's my current understanding at least.
(In reply to comment #9) > Comment on attachment 279023 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279023&action=review > > >>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2947 > >>>> + m_jit.move(baseTag, GPRInfo::argumentGPR3); > >>> > >>> This is wrong if basePayload/baseTag are aliased to argument registers w/ each other. > >>> i.e, if basePayload is argumentGPR3 and baseTag is argumentGPR2, this code will do the wrong thing. > >> > >> I thought this was not possible because the argumentGPR's are not in GPRInfo::toRegister() (and not counted in GPRInfo::numberOfRegisters). Including them might be an idea for the future though, as I suspect more registers available might improve performances (and ARM does that so it's probably possible), but I think that's a trickier change, and I'd like to get the build unbroken. > > > > I don't quite understand your response here. Tag/Payload regs will be allocated by the DFG's register allocator. It will happily use argument registers. > > I don't think this makes the code harder to read. All code we write that sets up a call frame must account for this. > > Sorry if that wasn't clear, and there is a possibility that I misunderstand > something. What I mean is that DFG::RegisterBank<GPRInfo>::allocate() (see > ::allocateInternal()) uses GPRInfo::toRegister() (and > GPRInfo::numberOfRegisters as RegisterBank::NUM_REGS) as its source of > registers. In the case of MIPS, GPRInfo::toRegister() never returns an > argumentGPRx. Therefore baseTag and basePayload cannot be an argumentGPRx > register. That's my current understanding at least. I see. Makes sense.
Comment on attachment 279023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279023&action=review Looks good to me >>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2931 >>> + m_jit.move(TrustedImm32(0), GPRInfo::argumentGPR1); >> >> Is this because 64-bit values need to be aligned on even registers? > > That was my reasoning. Though looking at the code generated by gcc for operationPutByValWithThisStrict(), even in -O0 it never accesses $a1 or the corresponding address on the stack, so I will remove that one line in a subsequent version of the patch. The next value (which is 64 bits) definitely needs to be on $a2 and $a3 though (and accordingly, 4 stack entries need to be reserved for the values we put in $a0-$a3, though they don't need to be set by the caller. I agree, there is no need to set $a1 to 0 here.
Created attachment 279395 [details] Patch New patch, not setting up $a1 and explaining the "alignment".
I spent some time trying to avoid the move, doing the following instead of setting up a JSValueOperand and moving its registers to $a2/$a3: VirtualRegister virtualRegister = m_jit.graph().varArgChild(node, 0)->virtualRegister(); m_jit.load32(JITCompiler::payloadFor(virtualRegister), GPRInfo::argumentGPR2); m_jit.load32(JITCompiler::tagFor(virtualRegister), GPRInfo::argumentGPR3); But then, when I run super-property-access.js, I get this: # jsc super-property-access.js Exception: TypeError: Attempted to assign to readonly property. baz@super-property-access.js:442:23 super-property-access.js:450:16 test@super-property-access.js:9:10 global code@super-property-access.js:429:5 # I spent some time investigating that but could not really understand what's wrong with that approach. So for now I think we can stick with the latest patch I posted.
Comment on attachment 279395 [details] Patch Clearing flags on attachment: 279395 Committed r201170: <http://trac.webkit.org/changeset/201170>
All reviewed patches have been landed. Closing bug.