Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2637583002:
[wasm] Fix codegen issue for i64.add and i64.sub on ia32 (Closed)
Description[wasm] Fix codegen issue for i64.add and i64.sub on ia32
The IA32AddPair and IA32SubPair instructions were using an input register as a
temporary value, which led to registers sometimes being clobbered when they
shouldn't have been. This led to problems, for example, in calling printf to
format doubles:
printf("%f", 1.2345) => 0.61725 (on x86)
BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5800
Review-Url: https://codereview.chromium.org/2637583002
Cr-Commit-Position: refs/heads/master@{#42486}
Committed: https://chromium.googlesource.com/v8/v8/+/037200e6250d5ea6fbd4f5071615c8c850e5a661
Patch Set 1 #Patch Set 2 : Add test for and fix i64.sub #Patch Set 3 : Add test-run-wasm-64.cc tests #
Total comments: 6
Patch Set 4 : Fixing nits #Patch Set 5 : Rebasing #
Messages
Total messages: 41 (22 generated)
The CQ bit was checked by eholk@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...
Description was changed from ========== Improve handling of i64.add on ia32 BUG= ========== to ========== [wasm] Fix codegen issue for i64.add and i64.sub on ia32 The IA32AddPair and IA32SubPair instructions were using an input register as a temporary value, which led to registers sometimes being clobbered when they shouldn't have been. This led to problems, for example, in calling printf to format doubles. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5800 ==========
eholk@chromium.org changed reviewers: + ahaas@chromium.org, bradnelson@chromium.org, titzer@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2017/01/17 at 22:40:05, bradnelson wrote: > lgtm lgtm, thank for fixing this issue.
The CQ bit was checked by bradnelson@google.com
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
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/32661)
Need titzer for src/compiler
On 2017/01/18 09:26:51, bradn wrote: > Need titzer for src/compiler Can you add a test-run-wasm-64.cc test for this? Since it's a low-level codegen issue.
PTAL I added the tests to test-run-wasm-64 as well.
eholk@chromium.org changed reviewers: + mtrofin@chromium.org
Description was changed from ========== [wasm] Fix codegen issue for i64.add and i64.sub on ia32 The IA32AddPair and IA32SubPair instructions were using an input register as a temporary value, which led to registers sometimes being clobbered when they shouldn't have been. This led to problems, for example, in calling printf to format doubles. BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5800 ========== to ========== [wasm] Fix codegen issue for i64.add and i64.sub on ia32 The IA32AddPair and IA32SubPair instructions were using an input register as a temporary value, which led to registers sometimes being clobbered when they shouldn't have been. This led to problems, for example, in calling printf to format doubles: printf("%f", 1.2345) => 0.61725 (on x86) BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5800 ==========
LGTM with nits https://codereview.chromium.org/2637583002/diff/40001/test/cctest/wasm/test-r... File test/cctest/wasm/test-run-wasm-64.cc (right): https://codereview.chromium.org/2637583002/diff/40001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm-64.cc:139: WASM_EXEC_TEST(I64AddRegress5800) { Elsewhere in cctest, we appear to have the naming pattern "Regressxyz_somethingelse". Example (albeit this uses lower case rather than "Regress" like everywhere else) ./test-assembler-arm.cc:TEST(regress4292_b) https://codereview.chromium.org/2637583002/diff/40001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm-64.cc:143: WASM_I64V(0), WASM_I64V(0x100000000)))), add comment explaining why you picked 0x100000000 maybe add a constant like kHasBit33On, and reuse in both tests. https://codereview.chromium.org/2637583002/diff/40001/test/mjsunit/regress/wa... File test/mjsunit/regress/wasm/regression-5800.js (right): https://codereview.chromium.org/2637583002/diff/40001/test/mjsunit/regress/wa... test/mjsunit/regress/wasm/regression-5800.js:15: kExprI64Const, 0x80, 0x80, 0x80, 0x80, 0x10, mind adding a comment explaining what's with these numbers? (leb of a 33rd bit flipped on)
https://codereview.chromium.org/2637583002/diff/40001/test/cctest/wasm/test-r... File test/cctest/wasm/test-run-wasm-64.cc (right): https://codereview.chromium.org/2637583002/diff/40001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm-64.cc:139: WASM_EXEC_TEST(I64AddRegress5800) { On 2017/01/18 21:01:56, Mircea Trofin wrote: > Elsewhere in cctest, we appear to have the naming pattern > "Regressxyz_somethingelse". > Example (albeit this uses lower case rather than "Regress" like everywhere else) > > ./test-assembler-arm.cc:TEST(regress4292_b) > Done. https://codereview.chromium.org/2637583002/diff/40001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm-64.cc:143: WASM_I64V(0), WASM_I64V(0x100000000)))), On 2017/01/18 21:01:56, Mircea Trofin wrote: > add comment explaining why you picked 0x100000000 > > maybe add a constant like kHasBit33On, and reuse in both tests. Done. https://codereview.chromium.org/2637583002/diff/40001/test/mjsunit/regress/wa... File test/mjsunit/regress/wasm/regression-5800.js (right): https://codereview.chromium.org/2637583002/diff/40001/test/mjsunit/regress/wa... test/mjsunit/regress/wasm/regression-5800.js:15: kExprI64Const, 0x80, 0x80, 0x80, 0x80, 0x10, On 2017/01/18 21:01:56, Mircea Trofin wrote: > mind adding a comment explaining what's with these numbers? (leb of a 33rd bit > flipped on) Done.
The CQ bit was checked by eholk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bradnelson@chromium.org, ahaas@chromium.org, mtrofin@chromium.org Link to the patchset: https://codereview.chromium.org/2637583002/#ps60001 (title: "Fixing nits")
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
Try jobs failed on following builders: v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)
The CQ bit was checked by eholk@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
Try jobs failed on following builders: v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)
The CQ bit was checked by eholk@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 checked by eholk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org, bradnelson@chromium.org, ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2637583002/#ps80001 (title: "Rebasing")
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
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...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by eholk@chromium.org
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": 80001, "attempt_start_ts": 1484786976397260,
"parent_rev": "9f545ea96f18b7036ac6ec43e359d63f41c3686a", "commit_rev":
"037200e6250d5ea6fbd4f5071615c8c850e5a661"}
Message was sent while issue was closed.
Description was changed from
==========
[wasm] Fix codegen issue for i64.add and i64.sub on ia32
The IA32AddPair and IA32SubPair instructions were using an input register as a
temporary value, which led to registers sometimes being clobbered when they
shouldn't have been. This led to problems, for example, in calling printf to
format doubles:
printf("%f", 1.2345) => 0.61725 (on x86)
BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5800
==========
to
==========
[wasm] Fix codegen issue for i64.add and i64.sub on ia32
The IA32AddPair and IA32SubPair instructions were using an input register as a
temporary value, which led to registers sometimes being clobbered when they
shouldn't have been. This led to problems, for example, in calling printf to
format doubles:
printf("%f", 1.2345) => 0.61725 (on x86)
BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5800
Review-Url: https://codereview.chromium.org/2637583002
Cr-Commit-Position: refs/heads/master@{#42486}
Committed:
https://chromium.googlesource.com/v8/v8/+/037200e6250d5ea6fbd4f5071615c8c850e...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/037200e6250d5ea6fbd4f5071615c8c850e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
