Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2662233002:
[wasm] Remove x / -1 = -x constant folding for wasm (Closed)
Description[wasm] Remove x / -1 = -x constant folding for wasm
TEST=mjsunit/wasm/float-constant-folding
R=titzer@chromium.org
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Messages
Total messages: 24 (16 generated)
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2662233002/diff/1/src/compiler/machine-operat... File src/compiler/machine-operator-reducer.cc (right): https://codereview.chromium.org/2662233002/diff/1/src/compiler/machine-operat... src/compiler/machine-operator-reducer.cc:440: if (!wasm_origin_ && m.right().Is(-1)) { // x / -1.0 => -x Can we rename this option to something closer to what it means, e.g. "preserve_nan_bits" or something similar? https://codereview.chromium.org/2662233002/diff/1/test/mjsunit/wasm/float-con... File test/mjsunit/wasm/float-constant-folding.js (right): https://codereview.chromium.org/2662233002/diff/1/test/mjsunit/wasm/float-con... test/mjsunit/wasm/float-constant-folding.js:192: kExprF64ReinterpretI64, Can't this just be a F64Const?
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
https://codereview.chromium.org/2662233002/diff/1/src/compiler/machine-operat... File src/compiler/machine-operator-reducer.cc (right): https://codereview.chromium.org/2662233002/diff/1/src/compiler/machine-operat... src/compiler/machine-operator-reducer.cc:440: if (!wasm_origin_ && m.right().Is(-1)) { // x / -1.0 => -x On 2017/01/31 at 17:24:49, titzer wrote: > Can we rename this option to something closer to what it means, e.g. "preserve_nan_bits" or something similar? Done with the rebasing. https://codereview.chromium.org/2662233002/diff/1/test/mjsunit/wasm/float-con... File test/mjsunit/wasm/float-constant-folding.js (right): https://codereview.chromium.org/2662233002/diff/1/test/mjsunit/wasm/float-con... test/mjsunit/wasm/float-constant-folding.js:192: kExprF64ReinterpretI64, On 2017/01/31 at 17:24:49, titzer wrote: > Can't this just be a F64Const? Actually no, for two reasons: 1) Other constant folding would apply, not the one I want to test. 2) There is no way to specify a signalling NaN in JavaScript.
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/31 at 19:43:37, ahaas wrote: > https://codereview.chromium.org/2662233002/diff/1/src/compiler/machine-operat... > File src/compiler/machine-operator-reducer.cc (right): > > https://codereview.chromium.org/2662233002/diff/1/src/compiler/machine-operat... > src/compiler/machine-operator-reducer.cc:440: if (!wasm_origin_ && m.right().Is(-1)) { // x / -1.0 => -x > On 2017/01/31 at 17:24:49, titzer wrote: > > Can we rename this option to something closer to what it means, e.g. "preserve_nan_bits" or something similar? > > Done with the rebasing. > > https://codereview.chromium.org/2662233002/diff/1/test/mjsunit/wasm/float-con... > File test/mjsunit/wasm/float-constant-folding.js (right): > > https://codereview.chromium.org/2662233002/diff/1/test/mjsunit/wasm/float-con... > test/mjsunit/wasm/float-constant-folding.js:192: kExprF64ReinterpretI64, > On 2017/01/31 at 17:24:49, titzer wrote: > > Can't this just be a F64Const? > > Actually no, for two reasons: > 1) Other constant folding would apply, not the one I want to test. > 2) There is no way to specify a signalling NaN in JavaScript. Ping
lgtm
The CQ bit was checked by ahaas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/compiler/machine-operator-reducer.cc:
While running git apply --index -p1;
error: patch failed: src/compiler/machine-operator-reducer.cc:437
error: src/compiler/machine-operator-reducer.cc: patch does not apply
Patch: src/compiler/machine-operator-reducer.cc
Index: src/compiler/machine-operator-reducer.cc
diff --git a/src/compiler/machine-operator-reducer.cc
b/src/compiler/machine-operator-reducer.cc
index
f7fe19d494cc3aafdfcc0117053434e649521d33..e59fe0775573f60fe7695368b20d8b9b6ed669b6
100644
--- a/src/compiler/machine-operator-reducer.cc
+++ b/src/compiler/machine-operator-reducer.cc
@@ -437,7 +437,7 @@ Reduction MachineOperatorReducer::Reduce(Node* node) {
if (m.IsFoldable()) { // K / K => K
return ReplaceFloat64(m.left().Value() / m.right().Value());
}
- if (m.right().Is(-1)) { // x / -1.0 => -x
+ if (allow_signalling_nan_ && m.right().Is(-1)) { // x / -1.0 => -x
node->RemoveInput(1);
NodeProperties::ChangeOp(node, machine()->Float64Neg());
return Changed(node);
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
On 2017/02/03 at 09:48:16, commit-bot wrote:
> Failed to apply patch for src/compiler/machine-operator-reducer.cc:
> While running git apply --index -p1;
> error: patch failed: src/compiler/machine-operator-reducer.cc:437
> error: src/compiler/machine-operator-reducer.cc: patch does not apply
>
> Patch: src/compiler/machine-operator-reducer.cc
> Index: src/compiler/machine-operator-reducer.cc
> diff --git a/src/compiler/machine-operator-reducer.cc
b/src/compiler/machine-operator-reducer.cc
> index
f7fe19d494cc3aafdfcc0117053434e649521d33..e59fe0775573f60fe7695368b20d8b9b6ed669b6
100644
> --- a/src/compiler/machine-operator-reducer.cc
> +++ b/src/compiler/machine-operator-reducer.cc
> @@ -437,7 +437,7 @@ Reduction MachineOperatorReducer::Reduce(Node* node) {
> if (m.IsFoldable()) { // K / K => K
> return ReplaceFloat64(m.left().Value() / m.right().Value());
> }
> - if (m.right().Is(-1)) { // x / -1.0 => -x
> + if (allow_signalling_nan_ && m.right().Is(-1)) { // x / -1.0 => -x
> node->RemoveInput(1);
> NodeProperties::ChangeOp(node, machine()->Float64Neg());
> return Changed(node);
Seems like I made a mistake with rebasing and committed this CL as part of
another CL. Sorry.
|
