Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647353007:
[wasm] Fix constant folding with signalling NaN. (Closed)
Description[wasm] Fix constant folding with signalling NaN.
According to the WebAssembly spec no arithmetic operation should ever
return a signalling NaN. With the constant folding in V8, however, it
was possible that some arithmetic operations were elided, and if the
input of the arithmetic operation was a signalling NaN, then also the
result was the same signalling NaN. This CL removes some constant
folding optimizations and adjusts others so that even with constant
folding the result of an arithmetic operation is never a signalling NaN.
R=titzer@chromium.org, rossberg@chromium.org, bmeurer@chromium.org
Review-Url: https://codereview.chromium.org/2647353007
Cr-Commit-Position: refs/heads/master@{#42694}
Committed: https://chromium.googlesource.com/v8/v8/+/55aed7821012bc4ffaeeea5d887587e2fc69560f
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add TODOs #Patch Set 3 : Fixed tests. #Patch Set 4 : Remove unused variable #
Total comments: 1
Depends on Patchset: Messages
Total messages: 33 (23 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...
LGTM.
lgtm https://codereview.chromium.org/2647353007/diff/1/src/compiler/machine-operat... File src/compiler/machine-operator-reducer.cc (right): https://codereview.chromium.org/2647353007/diff/1/src/compiler/machine-operat... src/compiler/machine-operator-reducer.cc:362: if (m.right().IsNaN()) { // x - NaN => NaN Can you leave a TODO here that we could still do the original strength reduction if we know the non-zero input was not a signaling NaN (which is AFAICT the case for normal JS code?)
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...
On 2017/01/25 at 13:28:57, titzer wrote: > lgtm > > https://codereview.chromium.org/2647353007/diff/1/src/compiler/machine-operat... > File src/compiler/machine-operator-reducer.cc (right): > > https://codereview.chromium.org/2647353007/diff/1/src/compiler/machine-operat... > src/compiler/machine-operator-reducer.cc:362: if (m.right().IsNaN()) { // x - NaN => NaN > Can you leave a TODO here that we could still do the original strength reduction if we know the non-zero input was not a signaling NaN (which is AFAICT the case for normal JS code?) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
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...
On 2017/01/25 at 13:47:18, ahaas wrote: > On 2017/01/25 at 13:28:57, titzer wrote: > > lgtm > > > > https://codereview.chromium.org/2647353007/diff/1/src/compiler/machine-operat... > > File src/compiler/machine-operator-reducer.cc (right): > > > > https://codereview.chromium.org/2647353007/diff/1/src/compiler/machine-operat... > > src/compiler/machine-operator-reducer.cc:362: if (m.right().IsNaN()) { // x - NaN => NaN > > Can you leave a TODO here that we could still do the original strength reduction if we know the non-zero input was not a signaling NaN (which is AFAICT the case for normal JS code?) > > Done. PTAL. The c++ compiler does the constant folding we try to avoid in the optdebug build. I do ReplaceFloat64(m.right().Value() - m.right().Value()); instead of ReplaceFloat64(m.right().Value() - 0.0); now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
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...
https://codereview.chromium.org/2647353007/diff/60001/src/compiler/machine-op... File src/compiler/machine-operator-reducer.cc (right): https://codereview.chromium.org/2647353007/diff/60001/src/compiler/machine-op... src/compiler/machine-operator-reducer.cc:325: // Do some calculation to make a signalling NaN quiet. Would it be correct to just return the canonical quiet NaN?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
On 2017/01/25 at 15:30:54, titzer wrote: > https://codereview.chromium.org/2647353007/diff/60001/src/compiler/machine-op... > File src/compiler/machine-operator-reducer.cc (right): > > https://codereview.chromium.org/2647353007/diff/60001/src/compiler/machine-op... > src/compiler/machine-operator-reducer.cc:325: // Do some calculation to make a signalling NaN quiet. > Would it be correct to just return the canonical quiet NaN? Fair enough, the canonical should work just fine, at least according to the WebAssembly spec.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:80001) has been deleted
On 2017/01/26 at 11:47:02, ahaas wrote: > On 2017/01/25 at 15:30:54, titzer wrote: > > https://codereview.chromium.org/2647353007/diff/60001/src/compiler/machine-op... > > File src/compiler/machine-operator-reducer.cc (right): > > > > https://codereview.chromium.org/2647353007/diff/60001/src/compiler/machine-op... > > src/compiler/machine-operator-reducer.cc:325: // Do some calculation to make a signalling NaN quiet. > > Would it be correct to just return the canonical quiet NaN? > > Fair enough, the canonical should work just fine, at least according to the WebAssembly spec. I take it back. I is true that according to the WebAssembly spec the canonical quiet NaN would be quite fine, but the spec tests we fail still require a specific bit pattern. We should eventually change the spec tests, but for now I think it is easier to preserve the bit pattern.
The CQ bit was checked by ahaas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org, titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2647353007/#ps60001 (title: "Remove unused variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485434273600220,
"parent_rev": "01d051ff33d9dbaa5d280a7a623e3c093c0e6bd4", "commit_rev":
"55aed7821012bc4ffaeeea5d887587e2fc69560f"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Fix constant folding with signalling NaN. According to the WebAssembly spec no arithmetic operation should ever return a signalling NaN. With the constant folding in V8, however, it was possible that some arithmetic operations were elided, and if the input of the arithmetic operation was a signalling NaN, then also the result was the same signalling NaN. This CL removes some constant folding optimizations and adjusts others so that even with constant folding the result of an arithmetic operation is never a signalling NaN. R=titzer@chromium.org, rossberg@chromium.org, bmeurer@chromium.org ========== to ========== [wasm] Fix constant folding with signalling NaN. According to the WebAssembly spec no arithmetic operation should ever return a signalling NaN. With the constant folding in V8, however, it was possible that some arithmetic operations were elided, and if the input of the arithmetic operation was a signalling NaN, then also the result was the same signalling NaN. This CL removes some constant folding optimizations and adjusts others so that even with constant folding the result of an arithmetic operation is never a signalling NaN. R=titzer@chromium.org, rossberg@chromium.org, bmeurer@chromium.org Review-Url: https://codereview.chromium.org/2647353007 Cr-Commit-Position: refs/heads/master@{#42694} Committed: https://chromium.googlesource.com/v8/v8/+/55aed7821012bc4ffaeeea5d887587e2fc6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/55aed7821012bc4ffaeeea5d887587e2fc6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
