[JSC] Use spoolers in FTL OSR exit thunk
Created attachment 455897 [details] Patch
Created attachment 455899 [details] Patch
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.
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.
Committed r292078 (249005@trunk): <https://commits.webkit.org/249005@trunk>
<rdar://problem/91009994>