[FTL] NewArrayBuffer should be sinked if it is only used for spreading
Created attachment 327056 [details] Patch
Comment on attachment 327056 [details] Patch Attachment 327056 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/5258059 New failing tests: wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory stress/array-indexof-structure-change-convert.js.ftl-no-cjit-small-pool stress/generator-fib-ftl-and-array.js.ftl-eager-no-cjit-b3o1 microbenchmarks/string-get-by-val-out-of-bounds-insane.js.ftl-eager-no-cjit-b3o1 microbenchmarks/string-get-by-val-out-of-bounds.js.default microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-eager wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.default-wasm stress/array-indexof-structure-change-convert.js.ftl-eager-no-cjit stress/array-indexof-structure-change-convert.js.ftl-no-cjit-no-inline-validate stress/array-indexof-structure-change-convert.js.ftl-eager-no-cjit-b3o1 microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-b3o1 microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-eager-no-cjit-b3o1 wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-no-inline-validate microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-eager-no-cjit stress/array-indexof-structure-change-convert.js.default microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-small-pool microbenchmarks/string-get-by-val-out-of-bounds-insane.js.ftl-eager microbenchmarks/string-get-by-val-out-of-bounds-insane.js.ftl-eager-no-cjit microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-validate-sampling-profiler stress/exception-in-to-property-key-should-be-handled-early.js.ftl-eager-no-cjit-b3o1 stress/array-indexof-structure-change-convert.js.ftl-no-cjit-validate-sampling-profiler wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-tls-context stress/array-indexof-structure-change-convert.js.ftl-eager stress/generator-fib-ftl-and-array.js.ftl-eager-no-cjit microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-no-put-stack-validate stress/array-indexof-structure-change-convert.js.ftl-no-cjit-no-put-stack-validate
(In reply to Build Bot from comment #2) > Comment on attachment 327056 [details] > Patch > > Attachment 327056 [details] did not pass jsc-ews (mac): > Output: http://webkit-queues.webkit.org/results/5258059 > > New failing tests: > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory > stress/array-indexof-structure-change-convert.js.ftl-no-cjit-small-pool > stress/generator-fib-ftl-and-array.js.ftl-eager-no-cjit-b3o1 > microbenchmarks/string-get-by-val-out-of-bounds-insane.js.ftl-eager-no-cjit- > b3o1 > microbenchmarks/string-get-by-val-out-of-bounds.js.default > microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-eager > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.default-wasm > stress/array-indexof-structure-change-convert.js.ftl-eager-no-cjit > stress/array-indexof-structure-change-convert.js.ftl-no-cjit-no-inline- > validate > stress/array-indexof-structure-change-convert.js.ftl-eager-no-cjit-b3o1 > microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-b3o1 > microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-eager-no-cjit-b3o1 > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes- > tls-context > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic > microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-no-inline- > validate > microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-eager-no-cjit > stress/array-indexof-structure-change-convert.js.default > microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-small-pool > microbenchmarks/string-get-by-val-out-of-bounds-insane.js.ftl-eager > microbenchmarks/string-get-by-val-out-of-bounds-insane.js.ftl-eager-no-cjit > microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-validate- > sampling-profiler > stress/exception-in-to-property-key-should-be-handled-early.js.ftl-eager-no- > cjit-b3o1 > stress/array-indexof-structure-change-convert.js.ftl-no-cjit-validate- > sampling-profiler > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-tls-context > stress/array-indexof-structure-change-convert.js.ftl-eager > stress/generator-fib-ftl-and-array.js.ftl-eager-no-cjit > microbenchmarks/string-get-by-val-out-of-bounds.js.ftl-no-cjit-no-put-stack- > validate > stress/array-indexof-structure-change-convert.js.ftl-no-cjit-no-put-stack- > validate Lol, it figures out the existing bug :)
Created attachment 327128 [details] Patch
Created attachment 327153 [details] Patch
Attachment 327153 [details] did not pass style-queue: ERROR: Source/WTF/wtf/FixedPointCombinator.h:38: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 327154 [details] Patch
Attachment 327154 [details] did not pass style-queue: ERROR: Source/WTF/wtf/FixedPointCombinator.h:38: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 327164 [details] Patch
Attachment 327164 [details] did not pass style-queue: ERROR: Source/WTF/wtf/FixedPointCombinator.h:38: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 327164 [details] Patch Attachment 327164 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5274421 New failing tests: http/tests/appcache/offline-access.html
Created attachment 327171 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 327289 [details] Patch
Attachment 327289 [details] did not pass style-queue: ERROR: Source/WTF/wtf/FixedPointCombinator.h:38: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 327289 [details] Patch Attachment 327289 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/5284537 New failing tests: stress/phantom-new-array-buffer-forward-varargs2.js.ftl-no-cjit-no-put-stack-validate stress/phantom-new-array-buffer-forward-varargs2.js.ftl-eager-no-cjit stress/phantom-new-array-buffer-forward-varargs2.js.ftl-eager-no-cjit-b3o1 stress/phantom-new-array-buffer-forward-varargs2.js.ftl-no-cjit-validate-sampling-profiler
Created attachment 327300 [details] Patch
Attachment 327300 [details] did not pass style-queue: ERROR: Source/WTF/wtf/FixedPointCombinator.h:38: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ping?
Comment on attachment 327300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327300&action=review r- because I think I found a bug w.r.t double representation. I'd like to see some tests for that if it's indeed a bug, and I'll re-review. Also some various other comments. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5110 > + if (use->child1()->op() == PhantomNewArrayBuffer) > + lengthCheck = m_out.speculateAdd(length, m_out.constInt32(use->child1()->numConstants())); This should be done as the "startLength" calculation above. You should also probably add an overflow check to that math to be extra safe. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5144 > + if (hasDouble(use->child1()->indexingType())) > + value = bitwise_cast<int64_t>(data[i].asNumber()); This is wrong. Please add a test for it. We're allocating a contiguous array here. We can not store doubles into it. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5150 > + index = m_out.add(index, m_out.constIntPtr(1)); Let's pull this out of the loop and make it: index = m_out.add(index, m_out.constIntPtr(use->child1()->numConstants()) > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7064 > + if (target->op() == PhantomNewArrayBuffer) > + length = m_out.constIntPtr(target->numConstants()); I think you can just add this to "numNonSpreadParameters". You may want to rename that variable to something like "staticArgumentCount" or something similar. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7219 > + value = bitwise_cast<int64_t>(data[i].asNumber()); ditto this is wrong. Please add tests. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7905 > + if (target->op() == PhantomNewArrayBuffer) { > + spreadLengths.append(m_out.constInt32(target->numConstants())); > + return; This could be considered as part of numberOfStaticArguments. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7953 > + if (hasDouble(target->indexingType())) > + value = bitwise_cast<int64_t>(data[i].asNumber()); Ditto. This is wrong. Please add a test > Source/WTF/wtf/FixedPointCombinator.h:33 > +struct FixedPointCombinator { I'm not really a fan of this name. There is noting requiring Function to reach a fixed point. Is there a reason you chose this name? I think a better name would have something referencing recursion in it.
Comment on attachment 327300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327300&action=review >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5110 >> + lengthCheck = m_out.speculateAdd(length, m_out.constInt32(use->child1()->numConstants())); > > This should be done as the "startLength" calculation above. You should also probably add an overflow check to that math to be extra safe. Nice, fixed. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5144 >> + value = bitwise_cast<int64_t>(data[i].asNumber()); > > This is wrong. Please add a test for it. We're allocating a contiguous array here. We can not store doubles into it. Nice, I'll fix this with a test. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5150 >> + index = m_out.add(index, m_out.constIntPtr(1)); > > Let's pull this out of the loop and make it: > index = m_out.add(index, m_out.constIntPtr(use->child1()->numConstants()) Fixed. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7064 >> + length = m_out.constIntPtr(target->numConstants()); > > I think you can just add this to "numNonSpreadParameters". You may want to rename that variable to something like "staticArgumentCount" or something similar. Sounds nice. Add target->numConstant() to numNonSpreadParameters, and rename it to staticArgumentCount. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7219 >> + value = bitwise_cast<int64_t>(data[i].asNumber()); > > ditto this is wrong. Please add tests. Fixed. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7905 >> + return; > > This could be considered as part of numberOfStaticArguments. Nice. Fixed. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7953 >> + value = bitwise_cast<int64_t>(data[i].asNumber()); > > Ditto. This is wrong. Please add a test Fixed. >> Source/WTF/wtf/FixedPointCombinator.h:33 >> +struct FixedPointCombinator { > > I'm not really a fan of this name. There is noting requiring Function to reach a fixed point. > > Is there a reason you chose this name? > > I think a better name would have something referencing recursion in it. I simply named it from this definition https://en.wikipedia.org/wiki/Fixed-point_combinator. But better name seems fine. I renamed this to `RecurableLambda`.
Created attachment 328083 [details] Patch
Attachment 328083 [details] did not pass style-queue: ERROR: Source/WTF/wtf/RecurableLambda.h:38: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 328083 [details] Patch Attachment 328083 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/5441764 New failing tests: stress/math-pow-with-never-NaN-exponent.js.ftl-eager stress/math-pow-with-never-NaN-exponent.js.ftl-eager-no-cjit-b3o1 stress/math-pow-with-never-NaN-exponent.js.ftl-eager-no-cjit microbenchmarks/call-spread-apply.js.ftl-eager-no-cjit-b3o1 stress/phantom-new-array-buffer-osr-exit.js.ftl-no-cjit-no-put-stack-validate stress/phantom-new-array-buffer-osr-exit.js.default stress/redundant-array-bounds-checks-addition.js.ftl-eager-no-cjit stress/get-by-val-out-of-bounds-basics.js.ftl-eager-no-cjit-b3o1 stress/redundant-array-bounds-checks-addition-skip-first.js.ftl-eager-no-cjit stress/phantom-new-array-buffer-osr-exit.js.ftl-no-cjit-no-inline-validate stress/ftl-try-catch-varargs-call-throws.js.ftl-eager-no-cjit-b3o1 stress/redundant-array-bounds-checks-addition-skip-first.js.ftl-eager-no-cjit-b3o1 stress/phantom-new-array-buffer-osr-exit.js.ftl-eager stress/phantom-new-array-buffer-osr-exit.js.ftl-no-cjit-b3o1 stress/pow-with-never-NaN-exponent.js.ftl-eager-no-cjit-b3o1 stress/redundant-array-bounds-checks-addition.js.ftl-eager-no-cjit-b3o1 stress/phantom-new-array-buffer-osr-exit.js.ftl-eager-no-cjit-b3o1 microbenchmarks/call-spread-apply.js.ftl-eager-no-cjit stress/phantom-new-array-buffer-osr-exit.js.ftl-no-cjit-validate-sampling-profiler stress/get-by-val-out-of-bounds-basics.js.ftl-eager-no-cjit stress/ftl-try-catch-varargs-call-throws.js.ftl-eager stress/pow-with-never-NaN-exponent.js.ftl-eager stress/pow-with-never-NaN-exponent.js.ftl-eager-no-cjit stress/redundant-array-bounds-checks-unchecked-addition.js.ftl-eager-no-cjit-b3o1 stress/ftl-try-catch-varargs-call-throws.js.ftl-eager-no-cjit stress/phantom-new-array-buffer-osr-exit.js.ftl-eager-no-cjit stress/redundant-array-bounds-checks-unchecked-addition.js.ftl-eager-no-cjit
Created attachment 328801 [details] Patch
Comment on attachment 328801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328801&action=review r=me Some comments and suggestions for tests for the cases where you may not have them. > Source/WTF/ChangeLog:8 > + We add RecurableLambda<>. This can take a lambda and offer a way This should be "RecursableLambda" everywhere in this patch. Both upper and lower case. > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:850 > + if (canConvertToStaticLoadStores(candidate)) { do you have tests for both this being true and false? > Source/JavaScriptCore/dfg/DFGValidate.cpp:763 > + VALIDATE((node), node->child1()->op() == PhantomCreateRest || node->child1()->op() == PhantomNewArrayBuffer); You need to update the Spread rule below to say we support Spread(PhantomNewArrayBuffer). It'd be great if you can write a test for it too. Maybe you can by looking at the code I added to originally support Spread(PhantomCreateRest) > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5194 > + // Because resulted array from NewArrayWithSpread is always contiguous, we should not generate value > + // in Double form even if PhantomNewArrayBuffer's indexingType is ArrayWithDouble. Did you add tests for this? > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5196 > + m_out.store64(m_out.constInt64(value), m_out.baseIndex(heap, storage, index, JSValue(), i * sizeof(JSValue))); Maybe it's worth being paranoid about (i*sizeof(JSValue)) overflowing here? We can just crash if it does. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5316 > + setJSValue(frozenPointer(m_node->child1()->cellOperand())); nice! Please add a test if possible as state in comment about Validation phase. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7296 > + auto emitArgumentsFromRightToLeft = recurableLambda([&](auto self, Node* target) -> void { Do you have tests that mix all three kinds of phantoms in a single call? PhantomCreateRest, PhantomNewArrayBuffer, and just normal values. If not please add. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7323 > + jit.subPtr(CCallHelpers::TrustedImmPtr(static_cast<size_t>(1)), scratchGPR2); I would vote for doing this outside the loop and adjusting the store below to do the right thing. > Source/WTF/wtf/RecurableLambda.h:33 > +class RecurableLambda { make this: RecursableLambda > Source/WTF/wtf/RecurableLambda.h:51 > +decltype(auto) recurableLambda(Functor&& f) make this: recursableLambda
Comment on attachment 328801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328801&action=review Thank you! >> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:850 >> + if (canConvertToStaticLoadStores(candidate)) { > > do you have tests for both this being true and false? Yeah. JSTests/stress/phantom-new-array-buffer-forward-varargs.js includes both true and false cases. >> Source/JavaScriptCore/dfg/DFGValidate.cpp:763 >> + VALIDATE((node), node->child1()->op() == PhantomCreateRest || node->child1()->op() == PhantomNewArrayBuffer); > > You need to update the Spread rule below to say we support Spread(PhantomNewArrayBuffer). It'd be great if you can write a test for it too. Maybe you can by looking at the code I added to originally support Spread(PhantomCreateRest) Oh, nice. Added the test supporting `Spread(PhantomNewArrayBuffer)`. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5194 >> + // in Double form even if PhantomNewArrayBuffer's indexingType is ArrayWithDouble. > > Did you add tests for this? Yes new-array-with-spread-double-new-array-buffer.js tests this behavior. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5196 >> + m_out.store64(m_out.constInt64(value), m_out.baseIndex(heap, storage, index, JSValue(), i * sizeof(JSValue))); > > Maybe it's worth being paranoid about (i*sizeof(JSValue)) overflowing here? We can just crash if it does. Added. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5316 >> + setJSValue(frozenPointer(m_node->child1()->cellOperand())); > > nice! Please add a test if possible as state in comment about Validation phase. Added spread-escapes-but-new-array-buffer-does-not.js. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7296 >> + auto emitArgumentsFromRightToLeft = recurableLambda([&](auto self, Node* target) -> void { > > Do you have tests that mix all three kinds of phantoms in a single call? PhantomCreateRest, PhantomNewArrayBuffer, and just normal values. If not please add. Yes, call-varargs-spread-new-array-buffer2.js is the thing. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7323 >> + jit.subPtr(CCallHelpers::TrustedImmPtr(static_cast<size_t>(1)), scratchGPR2); > > I would vote for doing this outside the loop and adjusting the store below to do the right thing. OK, fixed. >> Source/WTF/wtf/RecurableLambda.h:33 >> +class RecurableLambda { > > make this: RecursableLambda Fixed. >> Source/WTF/wtf/RecurableLambda.h:51 >> +decltype(auto) recurableLambda(Functor&& f) > > make this: recursableLambda Fixed.
Committed r226033: <https://trac.webkit.org/changeset/226033>
<rdar://problem/36103673>
Debug JSC bots are hitting an assertion on 59 tests after this change: ASSERTION FAILED: spread->child1()->op() == PhantomCreateRest https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/390
(In reply to Ryan Haddad from comment #30) > Debug JSC bots are hitting an assertion on 59 tests after this change: > ASSERTION FAILED: spread->child1()->op() == PhantomCreateRest > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/390 Yusuke, looks like you need to update PreciseLocalClobberize.
(In reply to Saam Barati from comment #31) > (In reply to Ryan Haddad from comment #30) > > Debug JSC bots are hitting an assertion on 59 tests after this change: > > ASSERTION FAILED: spread->child1()->op() == PhantomCreateRest > > https://build.webkit.org/builders/ > > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/390 > > Yusuke, looks like you need to update PreciseLocalClobberize. I will fix.
(In reply to Saam Barati from comment #32) > (In reply to Saam Barati from comment #31) > > (In reply to Ryan Haddad from comment #30) > > > Debug JSC bots are hitting an assertion on 59 tests after this change: > > > ASSERTION FAILED: spread->child1()->op() == PhantomCreateRest > > > https://build.webkit.org/builders/ > > > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/390 > > > > Yusuke, looks like you need to update PreciseLocalClobberize. > > I will fix. Should be fixed in: https://trac.webkit.org/changeset/226081/webkit
(In reply to Saam Barati from comment #33) > (In reply to Saam Barati from comment #32) > > (In reply to Saam Barati from comment #31) > > > (In reply to Ryan Haddad from comment #30) > > > > Debug JSC bots are hitting an assertion on 59 tests after this change: > > > > ASSERTION FAILED: spread->child1()->op() == PhantomCreateRest > > > > https://build.webkit.org/builders/ > > > > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/390 > > > > > > Yusuke, looks like you need to update PreciseLocalClobberize. > > > > I will fix. > > Should be fixed in: > https://trac.webkit.org/changeset/226081/webkit Oops! Thank you!