New issue
Advanced search Search tips

Issue 631318 link

Starred by 1 user
Status: Fixed
Owner:
bmeu...@chromium.org
Closed: Jul 2016
Cc:
jarin@chromium.org
bmeu...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

Dead-code elimination during representation selection is too aggressive

Project Member Reported by bmeu...@chromium.org, Jul 26 2016 
The following simple snippet illustrates the issue:

function foo(x) { return x < x; }
foo(1);
foo(2);
function bar(x) { foo(x); }
%OptimizeFunctionOnNextCall(bar);
assertThrows(() => bar(Symbol.toPrimitive));

We do eliminate the SpeculativeNumberLessThan node in this case, because it has no value uses once foo is inlined into bar. However, that way we also kill the input check on x, and thus do not raise an exception for the symbol input.
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/32346aaea037f23a6c312c323fcc8ba9673c22aa

commit 32346aaea037f23a6c312c323fcc8ba9673c22aa
Author: bmeurer <bmeurer@chromium.org>
Date: Tue Jul 26 07:07:32 2016

[turbofan] Fix overly aggressive dead code elimination.

When we eliminate nodes during truncation analysis that have no value
uses, we must make sure that we do not eliminate speculative number
operations that would have side effects depending on the inputs, i.e.
for example a SpeculativeNumberMultiply(x,y) does ToNumber(x) and
ToNumber(y) first, so if either x or y could throw an exception during
ToNumber conversion, we must not eliminate the multiplication, even if
it has no value uses (some later pass may kill the actual machine
multiplication, but the checks on the inputs have to remain still).
So we check whether both x and y are PlainPrimitive, i.e. neither
Receiver nor Symbol, which could raise exceptions for ToNumber, and
only in that case we propagate the "unusedness" of the node to its
inputs.

This also uncovered a bug with the type of Dead, which must be None,
as this represents an impossible value, so we had to fix that too.

Also the dead code removal will not work correctly for constants (i.e.
pure nodes with no value inputs), as those might be cached and hence
we might resurrect them for an unrelated node lowering during
SimplifiedLowering and only later kill the actual node (replacing its
uses with Dead), which would then also replace the new use with Dead.
So that was fixed as well. This shouldn't change anything for the
result, as unused constants automagically disappear from the graph later
on anyways.

R=yangguo@chromium.org
BUG= chromium:631318 

Review-Url: https://codereview.chromium.org/2182003002
Cr-Commit-Position: refs/heads/master@{#38038}

[modify] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/src/compiler/simplified-lowering.cc
[modify] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/src/compiler/typer.cc
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-1.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-10.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-11.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-12.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-13.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-14.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-15.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-2.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-3.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-4.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-5.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-6.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-7.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-8.js
[add] https://crrev.com/32346aaea037f23a6c312c323fcc8ba9673c22aa/test/mjsunit/regress/regress-crbug-631318-9.js

Comment 2 by bmeu...@chromium.org, Jul 26 2016

Status: Fixed (was: Started)
►

Sign in to add a comment