Bug 230622 – [JSC][32bit] Fix CSR restore on DFG tail calls, add extra register on ARMv7

• Component: New Bugs
• Status: RESOLVED FIXED

[Geza Lore]

Allow DFG to use regCS0 GPR (LLInt metadataTable) on ARMv7

[Geza Lore]

Created attachment 438945 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 438945 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:83
> +    loadp CodeBlock::m_metadata[PB], metadataTable

This looks incorrect to me since it is callee-save. If DFG uses it, then DFG should restore it, not LLInt.

[Geza Lore]

I see your argument, but PB is restored on the line below, which is also a callee save, and that restore is necessary as DFG doesn't restore that either. Is that restore of PB wrong as well in this case?

[Geza Lore]

Created attachment 439402 [details]
Patch

[Geza Lore]

Created attachment 439407 [details]
Patch

[Geza Lore]

Created attachment 439457 [details]
Patch

[Geza Lore]

Created attachment 439458 [details]
Patch

[Geza Lore]

Created attachment 439459 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83668548>

[Geza Lore]

Created attachment 440261 [details]
Patch

[Geza Lore]

Created attachment 440262 [details]
Patch

[Geza Lore]

Created attachment 440492 [details]
Patch

[Yusuke Suzuki]

@Saam Do you have an opinion on this change? I'm asking since you are working on unlinked DFG / FTL, so it is possible that metadata register can be used in DFG / FTL.

[Saam Barati]

Comment on attachment 440492 [details]
Patch

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

> Source/JavaScriptCore/bytecode/ValueRecovery.h:195
> +    static ValueRecovery calleeSaveRegDisplacedInJSStack(VirtualRegister virtualReg, bool inTag)

why is the implication here that we're dealing with an Int32?

[Geza Lore]

Comment on attachment 440492 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/ValueRecovery.h:195
>> +    static ValueRecovery calleeSaveRegDisplacedInJSStack(VirtualRegister virtualReg, bool inTag)
> 
> why is the implication here that we're dealing with an Int32?

On 32-bit platforms up to two 32-bit machine registers (which look like Int32) are stored in a single 64-bit VirtualRegister slot on the stack on procedure entry. Reusing the Int32 recovery mechanism takes care of restoring the one stored in the "payload" half of the VReg. I added the Int32Tag mechanism to restore the other 32-bit machine reg from the other half of the VReg.

[Saam Barati]

Comment on attachment 440492 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:3
> +        Allow DFG to use regCS0 GPR (LLInt metadataTable) on ARMv7

nit: this patch is really doing 2 things:
1. Allow DFG to use this register
2. Make sure tail calls properly restore CSRs in DFG.

I'd argue (2) is the really important part.

I'd make the title reflect (1) and (2) perhaps, or at least make mention of (2)

> Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:63
> +        if (saveSlotIndexInCPURegisters < 0)
> +            saveSlotIndexInCPURegisters -= 1; // Round towards -inf

I'm pretty confused as to what this is doing and why it's needed.

> Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:64
> +        VirtualRegister saveSlot { saveSlotIndexInCPURegisters / 2 };

Why /2?

[Geza Lore]

> I'm pretty confused as to what this is doing and why it's needed.
> 
> > Source/JavaScriptCore/jit/CallFrameShuffleData.cpp:64
> > +        VirtualRegister saveSlot { saveSlotIndexInCPURegisters / 2 };
> 
> Why /2?

there are two (32-bit) CPU register save slots per one (64-bit) virtual register slot on the stack. csr0/csr1 both need to map onto the first virual register slot (that's where the prologue puts them), but are in different halves (paload/tag). If we had csr2/csr3 would go into the second virtual register slot, hence the /2 (and the peculiar rounding bias above)

[Geza Lore]

Created attachment 441346 [details]
Patch

[Angelos Oikonomopoulos]

Created attachment 441386 [details]
[fast-cq] Patch

[Angelos Oikonomopoulos]

Created attachment 441388 [details]
[fast-cq] Patch

[Angelos Oikonomopoulos]

Unclear why the patch couldn't be landed via the commit-queue, even though analyze-layout-tests-results passes.

https://ews-build.webkit.org/#/builders/28/builds/16221)

"Patch 441346 is not marked cq+.".

Trying with [fast-cq] in case that makes a difference.

[EWS]

Committed r284255 (243064@main): <https://commits.webkit.org/243064@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441388 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 232353

[Geza Lore]

Created attachment 442578 [details]
Patch

[Geza Lore]

Fixed C_LOOP breakage on 32-bit (and removed superfluous restore on 64-bit non C_LOOP). Only differences from landed (and reverted) patch are in LowLevelInterpreter*.asm.

[Keith Miller]

Comment on attachment 442578 [details]
Patch

r=me

[EWS]

Committed r284923 (243594@main): <https://commits.webkit.org/243594@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442578 [details].