Bug 179762 – [FTL] NewArrayBuffer should be sinked if it is only used for spreading

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[FTL] NewArrayBuffer should be sinked if it is only used for spreading

[Yusuke Suzuki]

Created attachment 327056 [details]
Patch

[Build Bot]

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

[Yusuke Suzuki]

(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 :)

[Yusuke Suzuki]

Created attachment 327128 [details]
Patch

[Yusuke Suzuki]

Created attachment 327153 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 327154 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Created attachment 327164 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 327289 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 327300 [details]
Patch

[EWS Watchlist]

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.

[Yusuke Suzuki]

Ping?

[Saam Barati]

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.

[Yusuke Suzuki]

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`.

[Yusuke Suzuki]

Created attachment 328083 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 328801 [details]
Patch

[Yusuke Suzuki]

Ping?

[Saam Barati]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r226033: <https://trac.webkit.org/changeset/226033>

[Radar WebKit Bug Importer]

<rdar://problem/36103673>

[Ryan Haddad]

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

[Saam Barati]

(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.

[Saam Barati]

(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.

[Saam Barati]

(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

[Yusuke Suzuki]

(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!