Bug 238444 – [JSC] Use spoolers in FTL OSR exit thunk

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Use spoolers in FTL OSR exit thunk

[Yusuke Suzuki]

Created attachment 455897 [details]
Patch

[Yusuke Suzuki]

Created attachment 455899 [details]
Patch

[Mark Lam]

Comment on attachment 455899 [details]
Patch

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

Nice.  r=me.

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:389
> +                    if (!undefinedGPR) {

Use UNLIKELY?

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:400
> +                        if (!undefinedGPR) {

Use UNLIKELY?

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:503
> +            // The FTL compilation didn't preserve this register. This means that it also
> +            // didn't use the register. So its value at the beginning of OSR exit should be
> +            // preserved by the thunk. Luckily, we saved all registers into the register
> +            // scratch buffer, so we can restore them from there.

Since this now describe all the registers restorations below (as opposed to a single register as before), we should pluralized references to "register" in this comment.  I suggest (with some additional detail for added context):

unwindIndex == UINT_MAX indicates that the FTL compilation didn't preserve these registers.
This means that it also didn't use them. Their values at the beginning of OSR exit should be
the ones to retain. We saved all registers into the register scratch buffer at the beginning of
the thunk. So we can restore them from there.

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:521
> +                CCallHelpers::CopySpooler spooler(jit, GPRInfo::regT3, CCallHelpers::framePointerRegister, GPRInfo::regT0, GPRInfo::regT1, FPRInfo::fpRegT0, FPRInfo::fpRegT1);

nit: I suggest adding ASSERTs above the `jit.move(CCallHelpers::TrustedImmPtr(registerScratch), GPRInfo::regT3);` line above to ASSERT that regT0, refT1, and regT3 are not callee save since these loops include restoration of callee saves.  Hence, we can't use callee saves as temps.

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:542
> +            // The FTL compilation preserved the register. Its new value is therefore
> +            // irrelevant, but we can get the value that was preserved by using the unwind
> +            // data. We've already copied all unwind-able preserved registers into the unwind
> +            // scratch buffer, so we can get it from there.

Again, pluralize.  I suggest:

The FTL compilation preserved these registers. Their new values are therefore
irrelevant, but we can get their values that were preserved by using the unwind
data. We've already copied all unwind-able preserved registers into the unwind
scratch buffer, so we can get the values to restore from there.

[Yusuke Suzuki]

Comment on attachment 455899 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:389
>> +                    if (!undefinedGPR) {
> 
> Use UNLIKELY?

Nice. Added.

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:400
>> +                        if (!undefinedGPR) {
> 
> Use UNLIKELY?

Done.

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:503
>> +            // scratch buffer, so we can restore them from there.
> 
> Since this now describe all the registers restorations below (as opposed to a single register as before), we should pluralized references to "register" in this comment.  I suggest (with some additional detail for added context):
> 
> unwindIndex == UINT_MAX indicates that the FTL compilation didn't preserve these registers.
> This means that it also didn't use them. Their values at the beginning of OSR exit should be
> the ones to retain. We saved all registers into the register scratch buffer at the beginning of
> the thunk. So we can restore them from there.

Sounds good! Changed.

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:521
>> +                CCallHelpers::CopySpooler spooler(jit, GPRInfo::regT3, CCallHelpers::framePointerRegister, GPRInfo::regT0, GPRInfo::regT1, FPRInfo::fpRegT0, FPRInfo::fpRegT1);
> 
> nit: I suggest adding ASSERTs above the `jit.move(CCallHelpers::TrustedImmPtr(registerScratch), GPRInfo::regT3);` line above to ASSERT that regT0, refT1, and regT3 are not callee save since these loops include restoration of callee saves.  Hence, we can't use callee saves as temps.

Sounds good!

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:542
>> +            // scratch buffer, so we can get it from there.
> 
> Again, pluralize.  I suggest:
> 
> The FTL compilation preserved these registers. Their new values are therefore
> irrelevant, but we can get their values that were preserved by using the unwind
> data. We've already copied all unwind-able preserved registers into the unwind
> scratch buffer, so we can get the values to restore from there.

Nice, changed.

[Yusuke Suzuki]

Committed r292078 (249005@trunk): <https://commits.webkit.org/249005@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/91009994>