Bug 157741 – JSC: DFG::SpeculativeJIT::compile special case for MIPS for PutByValWithThis

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

[Guillaume Emont]

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.

[Guillaume Emont]

I am working on a patch.

[Guillaume Emont]

Created attachment 279023 [details]
Patch

A patch addressing the issue

[Guillaume Emont]

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.

[Saam Barati]

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?

[Konstantin Tokarev]

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

[Saam Barati]

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.

[Guillaume Emont]

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.

[Saam Barati]

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.

[Guillaume Emont]

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.

[Saam Barati]

(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.

[Julien Brianceau]

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.

[Guillaume Emont]

Created attachment 279395 [details]
Patch

New patch, not setting up $a1 and explaining the "alignment".

[Guillaume Emont]

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.                                                                                              
  [e-mail]:442:23                                                                                                                          
  super-property-access.js:450:16                                                                                                                              
  [e-mail]:9:10                                                                                                                           
  global [e-mail]: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.

[WebKit Commit Bot]

Comment on attachment 279395 [details]
Patch

Clearing flags on attachment: 279395

Committed r201170: <http://trac.webkit.org/changeset/201170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.