[webkit-reviews] review granted: [Bug 197993] Allow OSR exit to the LLInt : [Attachment 380168] patch

[bugzilla-daemon at webkit.org]

Tadeu Zagallo <tzagallo at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 197993: Allow OSR exit to the LLInt
https://bugs.webkit.org/show_bug.cgi?id=197993

Attachment 380168: patch

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




--- Comment #31 from Tadeu Zagallo <tzagallo at apple.com> ---
Comment on attachment 380168
  --> https://bugs.webkit.org/attachment.cgi?id=380168
patch

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

Looks good to me. I mostly just have nits about many things still being called
baseline, even when now referring to llint, and questions about code that I'm
not entirely familiar with.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:375
>	   ASSERT(baselineCodeBlock->jitType() == JITType::BaselineJIT);

How is this still correct?

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:810
> +	       CodeBlock* baselineCodeBlockForCaller =
baselineCodeBlockForOriginAndBaselineCodeBlock(*trueCaller,
outermostBaselineCodeBlock);

`baselineCodeBlockForOriginAndBaselineCodeBlock` doesn't seem like an accurate
name anymore

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:173
> +void* callerReturnPC(CodeBlock* baselineCodeBlockForCaller, unsigned
callBytecodeIndex, InlineCallFrame::Kind trueCallerCallKind, bool&
callerIsLLInt)

should `baselineCodeBlockForCaller` be called e.g. `codeBlockForCaller`?

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:331
> +	   if (callerIsLLInt) {

I'm not familiar with this code. Why is the exact same code duplicated in
DFGOSRExit.cpp?

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:40
> +#include "GetByIdMetadata.h"

Is this actually needed?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1738
> +    #.osrReturnPoint:

Not really relevant, but this is a bit weird... for a second I thought you had
invented some crazy syntax for magically declaring the osr exit points