[Webkit-unassigned] [Bug 131515] [ftlopt] Inlining native functions into the JavaScript in the FTL

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=131515


Filip Pizlo <fpizlo at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #232451|review+                     |review-
               Flag|                            |




--- Comment #46 from Filip Pizlo <fpizlo at apple.com>  2014-06-03 21:47:57 PST ---
(From update of attachment 232451)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.