[webkit-reviews] review denied: [Bug 131515] [ftlopt] Inlining native functions into the JavaScript in the FTL : [Attachment 232507] Native runtime library inlining in FTL for ftlopt branch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 4 15:48:07 PDT 2014
Filip Pizlo <fpizlo at apple.com> has denied Matthew Mirman <mmirman at apple.com>'s
request for review:
Bug 131515: [ftlopt] Inlining native functions into the JavaScript in the FTL
https://bugs.webkit.org/show_bug.cgi?id=131515
Attachment 232507: Native runtime library inlining in FTL for ftlopt branch
https://bugs.webkit.org/attachment.cgi?id=232507&action=review
------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232507&action=review
High-level comments:
- Please remove all changes to LayoutTests. I changed the tests before rolling
out this change. The changes I made were intentional.
- It would be better if you added all of the LLVM functions that you are using
the FTLAbbreviations.h, so that you can refer to them using the short form,
like setLinkage(global, LLVMPrivateLinkage) rather than
llvm->SetLinkage(global, LLVMPrivateLinkage). I would expect that
FTLLowerDFGToLLVM never uses llvm-> directly.
> LayoutTests/ChangeLog:21
> +2014-06-04 Matthew Mirman <mmirman at apple.com>
> +
> + [ftlopt] Added system for inlining native functions via the FTL.
> + https://bugs.webkit.org/show_bug.cgi?id=131515
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Fixes microbenchmarks.
> +
> + * js/regress/script-tests/ftl-library-inlining.js: Fixed bounds
> + * js/regress/script-tests/ftl-library-inlining-dataview.js: Fixed
Bounds
> + * js/regress/script-tests/ftl-library-inlining-exceptions.js: Added.
> + * js/regress/ftl-library-inlining-exceptions-expected.txt: Added.
> + * js/regress/ftl-library-inlining-exceptions.html: Added.
> + * js/regress/script-tests/ftl-library-inlining-folding.js: Added.
> + * js/regress/ftl-library-inlining-folding-expected.txt: Added.
> + * js/regress/ftl-library-inlining-folding-expected.html: Added.
> + * js/regress/script-tests/ftl-library-inlining-loops.js: Added.
> + * js/regress/ftl-library-inlining-loops-expected.txt: Added.
> + * js/regress/ftl-library-inlining-loops.html: Added.
> +
Please revert.
> LayoutTests/ChangeLog:-68
> - * js/regress/script-tests/ftl-library-inlining-exceptions.js: Added.
> - * js/regress/ftl-library-inlining-exceptions-expected.txt: Added.
> - * js/regress/ftl-library-inlining-exceptions.html: Added.
> - * js/regress/script-tests/ftl-library-inlining-folding.js: Added.
> - * js/regress/ftl-library-inlining-folding-expected.txt: Added.
> - * js/regress/ftl-library-inlining-folding-expected.html: Added.
> - * js/regress/script-tests/ftl-library-inlining-loops.js: Added.
> - * js/regress/ftl-library-inlining-loops-expected.txt: Added.
> - * js/regress/ftl-library-inlining-loops.html: Added.
Revert.
> LayoutTests/js/regress/ftl-library-inlining-exceptions-expected.txt:10
> +JSRegress/ftl-library-inlining-exceptions
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".
> +
> +
> +PASS no exception thrown
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +
Please revert.
> LayoutTests/js/regress/ftl-library-inlining-exceptions.html:12
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<html>
> +<head>
> +<script src="../../resources/js-test-pre.js"></script>
> +</head>
> +<body>
> +<script src="../../resources/regress-pre.js"></script>
> +<script src="script-tests/ftl-library-inlining-exceptions.js"></script>
> +<script src="../../resources/regress-post.js"></script>
> +<script src="../../resources/js-test-post.js"></script>
> +</body>
> +</html>
Revert.
> LayoutTests/js/regress/ftl-library-inlining-folding-expected.txt:10
> +JSRegress/ftl-library-inlining-folding
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".
> +
> +
> +PASS no exception thrown
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +
Revert.
> LayoutTests/js/regress/ftl-library-inlining-folding.html:12
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<html>
> +<head>
> +<script src="../../resources/js-test-pre.js"></script>
> +</head>
> +<body>
> +<script src="../../resources/regress-pre.js"></script>
> +<script src="script-tests/ftl-library-inlining-folding.js"></script>
> +<script src="../../resources/regress-post.js"></script>
> +<script src="../../resources/js-test-post.js"></script>
> +</body>
> +</html>
Revert.
> LayoutTests/js/regress/ftl-library-inlining-loops-expected.txt:10
> +JSRegress/ftl-library-inlining-loops
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".
> +
> +
> +PASS no exception thrown
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +
Revert.
> LayoutTests/js/regress/ftl-library-inlining-loops.html:12
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<html>
> +<head>
> +<script src="../../resources/js-test-pre.js"></script>
> +</head>
> +<body>
> +<script src="../../resources/regress-pre.js"></script>
> +<script src="script-tests/ftl-library-inlining-loops.js"></script>
> +<script src="../../resources/regress-post.js"></script>
> +<script src="../../resources/js-test-post.js"></script>
> +</body>
> +</html>
Revert.
> LayoutTests/js/regress/script-tests/ftl-library-inlining-dataview.js:13
> (function() {
> var result = 0;
> var d = new DataView(new ArrayBuffer(5));
> - for (var i = 0; i < 1000000; i++) {
> + for (var i = 0; i < 400000; i++) {
> d.setInt8(0, 4);
> d.setInt8(1, 2);
> d.setInt8(2, 6);
> d.setInt16(0, 20);
> result += d.getInt8(2) + d.getInt8(0);
> }
> - if (result != 6000000)
> + if (result != 2400000)
> throw "Bad result: " + result;
> -})();
> +})();
Revert.
> LayoutTests/js/regress/script-tests/ftl-library-inlining-exceptions.js:19
> +function foo(d){
> + return Date.prototype.getTimezoneOffset.call(d);
> +}
> +
> +noInline(foo);
> +
> +var x;
> +var count = 200000;
> +for (var i = 0 ; i < count; i++){
> + try {
> + foo(i < count - 1000 ? new Date() : "a");
> + x = false;
> + } catch (e) {
> + x = true;
> + }
> +}
> +
> +if (!x)
> + throw "bad result: "+ x;
Revert.
> LayoutTests/js/regress/script-tests/ftl-library-inlining-folding.js:18
> +function foo(){
> + var d = new DataView(new ArrayBuffer(5));
> + d.setInt8(0, 4);
> + d.setInt8(1, 2);
> + d.setInt8(2, 6);
> + d.setInt16(0, 20);
> + return d.getInt8(2) + d.getInt8(0);
> +}
> +
> +noInline(foo);
> +
> +var result = 0;
> +for (var i = 0 ; i < 300000; i++){
> + result += foo();
> +}
> +
> +if (result != 1800000)
> + throw "Bad result: " + result;
Revert.
> LayoutTests/js/regress/script-tests/ftl-library-inlining-loops.js:28
> +function foo(){
> + var count = 100;
> + var d = new DataView(new ArrayBuffer(count));
> +
> + for (var i = 0; i < count / 4; i++){
> + d.setInt32(i, i);
> + }
> +
> + for (var i = 0; i < count; i++){
> + d.setInt8(i, i);
> + }
> + var result = 0;
> + for (var i = 0; i < count; i++){
> + result += d.getInt8(i);
> + }
> + return result;
> +}
> +
> +noInline(foo);
> +
> +var r = 0;
> +for (var i = 0 ; i < 100000; i++){
> + r += foo();
> +}
> +
> +if (r != 495000000)
> + throw "Bad result: " + r;
> +
Revert.
> LayoutTests/js/regress/script-tests/ftl-library-inlining.js:5
> (function() {
> - for (var i = 0 ; i < 10000000; i++)
> - Math.random();
> + d = new Date();
> + for (var i = 0 ; i < 200000; i++)
> + Math.random() + d.getTimezoneOffset();
> })();
Revert.
> Source/JavaScriptCore/ChangeLog:-541
> -2014-06-03 Matthew Mirman <mmirman at apple.com>
> -
> - [ftlopt] Added system for inlining native functions via the FTL.
> - https://bugs.webkit.org/show_bug.cgi?id=131515
> -
> - Reviewed by Filip Pizlo.
> -
> - Also fixed the build to not compress the bitcode and to
> - include all of the relevant runtime. With
GCC_GENERATE_DEBUGGING_SYMBOLS = NO,
> - the produced bitcode files are a 100th the size they were before.
> - Now we can include all of the relevant runtime files with only a 3mb
overhead.
> - This is the same overhead as for two compressed files before,
> - but done more efficiently (on both ends) and with less code.
> -
> - Deciding whether to inline native functions is left up to LLVM.
> - The entire module containing the function is linked into the current
> - compiled JS so that inlining the native functions shouldn't make
them smaller.
> -
> - 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.
> -
> - * JavaScriptCore.xcodeproj/project.pbxproj: Added back runtime files
to compile.
> - * build-symbol-table-index.py: Changed bitcode suffix.
> - Added inclusion of only tested symbols.
> - Added output to InlineRuntimeSymbolTable.h.
> - * build-symbol-table-index.sh: Changed bitcode suffix.
> - * copy-llvm-ir-to-derived-sources.sh: Removed gzip compression.
> - * tested-symbols.symlst: Added.
> - * dfg/DFGByteCodeParser.cpp:
> - (JSC::DFG::ByteCodeParser::handleCall):
> - Now sets the knownFunction of the call node if such a function
exists
> - and emits a check that during runtime the callee is in fact known.
> - * dfg/DFGNode.h:
> - Added functions to set the known function of a call node.
> - (JSC::DFG::Node::canBeKnownFunction): Added.
> - (JSC::DFG::Node::hasKnownFunction): Added.
> - (JSC::DFG::Node::knownFunction): Added.
> - (JSC::DFG::Node::giveKnownFunction): Added.
> - * ftl/FTLAbbreviatedTypes.h: Added a typedef for LLVMMemoryBufferRef
> - * ftl/FTLLowerDFGToLLVM.cpp:
> - (JSC::FTL::LowerDFGToLLVM::isInlinableSize): Added. Hardcoded
threshold to 275.
> - (JSC::FTL::LowerDFGToLLVM::getModuleByPathForSymbol): Added.
> - (JSC::FTL::LowerDFGToLLVM::getFunctionBySymbol): Added.
> - (JSC::FTL::LowerDFGToLLVM::possiblyCompileInlineableNativeCall):
Added.
> - (JSC::FTL::LowerDFGToLLVM::compileCallOrConstruct):
> - Added call to possiblyCompileInlineableNativeCall
> - * ftl/FTLOutput.h:
> - (JSC::FTL::Output::allocaName): Added. Useful for debugging.
> - * ftl/FTLState.cpp:
> - (JSC::FTL::State::State): Added an include for
InlineRuntimeSymbolTable.h
> - * ftl/FTLState.h: Added symbol table hash table.
> - * ftl/FTLCompile.cpp:
> - (JSC::FTL::compile): Added inlining and dead function elimination
passes.
> - * heap/HandleStack.h: Added JS_EXPORT_PRIVATE to a few functions to
get inlining to compile.
> - * InlineRuntimeSymbolTable.h: Added.
> - * llvm/InitializeLLVMMac.mm: Deleted.
> - * llvm/InitializeLLVMMac.cpp: Added.
> - * llvm/LLVMAPIFunctions.h: Added macros to include Bitcode parsing
and linking functions.
> - * llvm/LLVMHeaders.h: Added includes for Bitcode parsing and
linking.
> - * runtime/BundlePath.h: Added.
> - * runtime/BundlePath.mm: Added.
> - * runtime/DateInstance.h: Added JS_EXPORT_PRIVATE to a few functions
to get inlining to compile.
> - * runtime/DateInstance.h: ditto.
> - * runtime/DateConversion.h: ditto.
> - * runtime/ExceptionHelpers.h: ditto.
> - * runtime/JSCJSValue.h: ditto.
> - * runtime/JSArray.h: ditto.
> - * runtime/JSDateMath.h: ditto.
> - * runtime/JSObject.h: ditto.
> - * runtime/JSObject.h: ditto.
> - * runtime/RegExp.h: ditto.
> - * runtime/Structure.h: ditto.
> - * runtime/Options.h: Added
maximumLLVMInstructionCountForNativeInlining.
> - * tests/stress/ftl-library-inlining-random.js: Added.
> - * tests/stress/ftl-library-substring.js: Added.
> -
Revert.
> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4104
> +#if ASSERT_DISABLED
> + llvm->RemoveFunctionAttr(function,
LLVMStackProtectAttribute);
> +#endif // ASSERT_DISABLED
Please use a normal if statement like:
if (ASSERT_DISABLED)
llvm->RemoveFunctionAttr(function, LLVMStackProtectAttribute);
More information about the webkit-reviews
mailing list