[webkit-reviews] review denied: [Bug 131515] [ftlopt] Inlining native functions into the JavaScript in the FTL : [Attachment 232451] Native runtime library inlining in FTL for ftlopt branch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied  review:
Bug 131515: [ftlopt] Inlining native functions into the JavaScript in the FTL
https://bugs.webkit.org/show_bug.cgi?id=131515

Attachment 232451: Native runtime library inlining in FTL for ftlopt branch
https://bugs.webkit.org/attachment.cgi?id=232451&action=review

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


> Source/JavaScriptCore/ChangeLog:23
> +	   Rather than loading Runtime.symtbl at runtime FTLState.cpp now
includes a file 
> +	   InlineRuntimeSymbolTable.h which statically builds the symbol table
hash table.  
> +	   Currently build-symbol-table-index.py updates this file from the 
> +	   contents of tested-symbols.symlst when done building as a matter of
convenience.  
> +	   However, in order to include the new contents of the file in the
build
> +	   you'd need to build twice.  This will be fixed in future versions.

We shouldn't do this.  Sorry, I hadn't realized the full effect of this.  Every
time someone builds, they will get a different InlineRuntimeSymbolTable.h and
it will show up in their diff.	This pretty much breaks the typical WebKit
development workflow.  Things that are checked into the repository shouldn't be
automagically mutated by the build system.  Can you remove this feature and
post a new patch?

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3677
> +#if CPU(X86_64)
> +	       "/Resources/Runtime/x86_64/",
> +#elif CPU(ARM64) 
> +	       "/Resources/Runtime/arm64/",
> +#else
> +#error "Unrecognized Architecture"
> +#endif

You should find a way of saying this that doesn't involve preprocessor #if's. 
Notice that we have isX86() and isARM64() inline functions in
AbstractMacroAssembler.h, and the FTL tends to use those whenever possible.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:3733
> +#if ASSERT_DISABLED
> +		   llvm->RemoveFunctionAttr(function,
LLVMStackProtectAttribute);
> +#endif // ASSERT_DISABLED

Change this to just "if (ASSERT_DISABLED) llvm->RemoveFunctionAttr(function,
LLVMStackProtectAttribute);" (on two lines, not one, of course).  That's the
style elsewhere in this file.

> Source/JavaScriptCore/ftl/FTLState.cpp:43
> +#define LINE_SIZE 500

No need to use #define.  Use "static const unsigned lineSize = 500".  Also, I'm
not sure where you're using this?