[webkit-reviews] review granted: [Bug 238444] [JSC] Use spoolers in FTL OSR exit thunk : [Attachment 455899] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 238444: [JSC] Use spoolers in FTL OSR exit thunk
https://bugs.webkit.org/show_bug.cgi?id=238444

Attachment 455899: Patch

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




--- Comment #3 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 455899
  --> https://bugs.webkit.org/attachment.cgi?id=455899
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.