[webkit-reviews] review denied: [Bug 131515] Inlining native functions into the JavaScript in the FTL : [Attachment 229180] System for inlining native functions via the FTL.

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Matthew Mirman <mmirman at apple.com>'s
request for review:
Bug 131515: Inlining native functions into the JavaScript in the FTL
https://bugs.webkit.org/show_bug.cgi?id=131515

Attachment 229180: System for inlining native functions via the FTL.  
https://bugs.webkit.org/attachment.cgi?id=229180&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229180&action=review


Does this have heuristics for deciding not to inline functions that are too
big?  

What is the performance impact?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:126
> +
> +

I don't think we should have a double-space here.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3560
> +	       LType tyCallee;

"tyCallee" isn't a very good variable name.  We don't usually use abbreviations
in WebKit.  "calleeType" would be better.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3584
> +		   llvm->GetParamTypes(tyCallee, &tyCalleeArg);
> +		   LValue call = m_out.call(callee, 
> +		       m_out.bitCast(m_out.address(m_execState,
m_heaps.CallFrame_callerFrame ).value(), tyCalleeArg));

This doesn't actually inline, right?  If so, you should either change the name
of this bug, or implement the inlining.  That probably involves adding some
inlining phases to the LLVM pipeline in FTLCompile.cpp.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3601
> +		   return;

This should be indented 4 fewer spaces.

> Source/JavaScriptCore/ftl/FTLOutput.h:213
> +    LValue allocaName(LType type, const char* nm) { return
llvm->BuildAlloca(m_builder, type, nm); }

Why do you need to use the name?  We usually don't name LLVM instructions. 
Also, it seems like you don't use this.