[webkit-reviews] review granted: [Bug 232746] [JSC] Use CallLinkInfo in LLInt : [Attachment 444130] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 232746: [JSC] Use CallLinkInfo in LLInt
https://bugs.webkit.org/show_bug.cgi?id=232746

Attachment 444130: Patch

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




--- Comment #22 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 444130
  --> https://bugs.webkit.org/attachment.cgi?id=444130
Patch

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

Nice patch, I like this direction in architecture. r=me

> Source/JavaScriptCore/jit/Repatch.cpp:1855
> +    // 2. If it is tail-call, linkRegister is not pointing the doneLocation
for slow-call case.

nit: Maybe write "3. If we're a data IC, then the return address is already
correct"

> Source/JavaScriptCore/jit/Repatch.cpp:1861
> +	   ASSERT(isDataIC);

nit: not sure we really need this assert given the above branch

Also, can we assert we're not DFG/FTL code given below returnFromBaseline stub.

> Source/JavaScriptCore/jit/Repatch.cpp:1865
> +	   if (callLinkInfo.isTailCall()) {
> +	      
stubJit.move(CCallHelpers::TrustedImmPtr(vm.getCTIStub(JIT::returnFromBaselineG
enerator).code().untaggedExecutableAddress()), GPRInfo::regT4);
> +	       stubJit.restoreReturnAddressBeforeReturn(GPRInfo::regT4);
> +	   }

Can we put a FIXME here maybe? Based on what we talked about on Slack. Since
tail calls are relying on returning here, it seems like when we take the slow
path we're not doing real tail calls. So maybe it's something we should fix.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:2557
> +	   move NotCellMask, t1
> +	   btqnz t0, t1, slowCase

this can't be one insn?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2251
> +	   # Those are r0 and r1

what is "Those" here?