Bug 227052 – AssemblyHelpers should save/restore callee save FPRs

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

[Tadeu Zagallo]

<rdar://77080162>

[Tadeu Zagallo]

Created attachment 431496 [details]
Patch

[Mark Lam]

Comment on attachment 431496 [details]
Patch

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

> Source/JavaScriptCore/jit/AssemblyHelpers.h:344
> +            if (entry.reg().isGPR())
> +                storePtr(entry.reg().gpr(), Address(framePointerRegister, entry.offset()));
> +            else
> +                storeDouble(entry.reg().fpr(), Address(framePointerRegister, entry.offset()));

You can just do:
    storeReg(entry.reg(), Address(framePointerRegister, entry.offset()));

> Source/JavaScriptCore/jit/AssemblyHelpers.h:383
> +                if (entry.reg().isGPR())
> +                    storePtr(entry.reg().gpr(), Address(framePointerRegister, offsetVirtualRegister.offsetInBytes() + entry.offset()));
> +                else
> +                    storeDouble(entry.reg().fpr(), Address(framePointerRegister, offsetVirtualRegister.offsetInBytes() + entry.offset()));
> +            }

Pretty sure this is not needed because this function is only ever called with a baseline CodeBlock.  Please check if I'm wrong.  If it is baseline CodeBlock only, then just RELEASE_ASSERT the Codeblock JITType at the top.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:407
> +            if (entry.reg().isGPR())
> +                loadPtr(Address(framePointerRegister, entry.offset()), entry.reg().gpr());
> +            else
> +                loadDouble(Address(framePointerRegister, entry.offset()), entry.reg().fpr());

Just use loadReg(Address(framePointerRegister, entry.offset()), entry.reg());

[Tadeu Zagallo]

Created attachment 431498 [details]
Patch

[Tadeu Zagallo]

Created attachment 431499 [details]
Patch

[Mark Lam]

Comment on attachment 431499 [details]
Patch

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

r=me with ChangeLog fixes.

> Source/JavaScriptCore/ChangeLog:9
> +        We have 3 functions in AssemblyHelpers to save and restore callee save registers that were filtering

4 functions (according to this patch), not 3.

> Source/JavaScriptCore/ChangeLog:10
> +        out any FPRs. This is an issue since we do have callee save FPRs in arm64 and these helpers can be

/these helpers can be/there are helpers/?  Otherwise, it's not clear which helpers the "these helpers" refer to.

> Source/JavaScriptCore/ChangeLog:11
> +        called from the FTL, which uses those callee saves. The test case shows how that's an issue with tail

remove the ',' to go with the above edit?

[Tadeu Zagallo]

Comment on attachment 431499 [details]
Patch

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

Thanks for the reviews, I'll reword the changelog.

>> Source/JavaScriptCore/ChangeLog:9
>> +        We have 3 functions in AssemblyHelpers to save and restore callee save registers that were filtering
> 
> 4 functions (according to this patch), not 3.

There are 3 functions in AssemblyHelpers and one in DFGOSREntry, which I mention below.

>> Source/JavaScriptCore/ChangeLog:10
>> +        out any FPRs. This is an issue since we do have callee save FPRs in arm64 and these helpers can be
> 
> /these helpers can be/there are helpers/?  Otherwise, it's not clear which helpers the "these helpers" refer to.

I think the issue is because I edited the other part of the phrase, but forgot about this one. I meant to say "these functions", referring to the "3 functions" from the previous sentence.

[Mark Lam]

Comment on attachment 431499 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:9
>>> +        We have 3 functions in AssemblyHelpers to save and restore callee save registers that were filtering
>> 
>> 4 functions (according to this patch), not 3.
> 
> There are 3 functions in AssemblyHelpers and one in DFGOSREntry, which I mention below.

ok.

>>> Source/JavaScriptCore/ChangeLog:10
>>> +        out any FPRs. This is an issue since we do have callee save FPRs in arm64 and these helpers can be
>> 
>> /these helpers can be/there are helpers/?  Otherwise, it's not clear which helpers the "these helpers" refer to.
> 
> I think the issue is because I edited the other part of the phrase, but forgot about this one. I meant to say "these functions", referring to the "3 functions" from the previous sentence.

ok.

[Tadeu Zagallo]

Created attachment 431549 [details]
Patch for landing

[EWS]

Committed r278937 (238868@main): <https://commits.webkit.org/238868@main>

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