Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2539533005:
[Turbofan] Modify ARM vswp assembler test to use integers. (Closed)
Description[Turbofan] Modify ARM vswp assembler test to use integers.
Attempt to fix or get insight into failing vswp test on V8 ARM bot.
LOG=N
BUG=
Committed: https://crrev.com/9e3feefff2fa2740515e38b50d0908588f0fb49e
Cr-Commit-Position: refs/heads/master@{#41397}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review comments. #
Total comments: 2
Patch Set 3 : Use different test pattern for q-reg swap to detect crosstalk. #Messages
Total messages: 28 (18 generated)
The CQ bit was checked by bbudge@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...
bbudge@chromium.org changed reviewers: + bmeurer@chromium.org, martyn.capewell@arm.com
Link to failing build: https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm%20-%20debug/bu...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jacob.bramley@arm.com changed reviewers: + jacob.bramley@arm.com
https://codereview.chromium.org/2539533005/diff/1/test/cctest/test-assembler-... File test/cctest/test-assembler-arm.cc (right): https://codereview.chromium.org/2539533005/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-arm.cc:2937: __ mov(r6, Operand(0xBFF00000)); `double_to_rawbits` could make this cleaner: uint64_t one = double_to_rawbits(1.0); __ mov(r5, Operand(one >> 32)); __ mov(r6, Operand(one & 0xffffffff)); https://codereview.chromium.org/2539533005/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-arm.cc:2977: CHECK_EQ(0xBFF0000000000000u, t.result0); Nearby tests use lower-case hexadecimal. This should probably be consistent. Also, instead of 'u', it'd be better to use UINT64_C(...), like in the unaligned_stores test above.
The CQ bit was checked by bbudge@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/2539533005/diff/1/test/cctest/test-assembler-... File test/cctest/test-assembler-arm.cc (right): https://codereview.chromium.org/2539533005/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-arm.cc:2937: __ mov(r6, Operand(0xBFF00000)); On 2016/11/30 10:41:08, jbramley wrote: > `double_to_rawbits` could make this cleaner: > > uint64_t one = double_to_rawbits(1.0); > __ mov(r5, Operand(one >> 32)); > __ mov(r6, Operand(one & 0xffffffff)); > I used bit_cast<uint64_t> since double_to_rawbits is only an arm64 function. https://codereview.chromium.org/2539533005/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-arm.cc:2977: CHECK_EQ(0xBFF0000000000000u, t.result0); On 2016/11/30 10:41:08, jbramley wrote: > Nearby tests use lower-case hexadecimal. This should probably be consistent. > > Also, instead of 'u', it'd be better to use UINT64_C(...), like in the > unaligned_stores test above. Or we can use the 'one' and 'minus_one' constants we define above. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM (with one comment) https://codereview.chromium.org/2539533005/diff/20001/test/cctest/test-assemb... File test/cctest/test-assembler-arm.cc (right): https://codereview.chromium.org/2539533005/diff/20001/test/cctest/test-assemb... test/cctest/test-assembler-arm.cc:2990: CHECK_EQ(one, t.result7); I wonder if it's useful to use other values (like the old test) in case we've crossed things over strangely.
The CQ bit was checked by bbudge@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/2539533005/diff/20001/test/cctest/test-assemb... File test/cctest/test-assembler-arm.cc (right): https://codereview.chromium.org/2539533005/diff/20001/test/cctest/test-assemb... test/cctest/test-assembler-arm.cc:2990: CHECK_EQ(one, t.result7); On 2016/11/30 17:38:05, jbramley wrote: > I wonder if it's useful to use other values (like the old test) in case we've > crossed things over strangely. Yes, good point. I'll put a non-FP test pattern in the q-register swap.
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 bbudge@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jacob.bramley@arm.com Link to the patchset: https://codereview.chromium.org/2539533005/#ps40001 (title: "Use different test pattern for q-reg swap to detect crosstalk.")
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": 40001, "attempt_start_ts": 1480534983527570,
"parent_rev": "e5bda2052754f0644cc4e349d92be9543aefa95e", "commit_rev":
"51c40417edb519d64873945fa859aff3af9f2802"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Turbofan] Modify ARM vswp assembler test to use integers. Attempt to fix or get insight into failing vswp test on V8 ARM bot. LOG=N BUG= ========== to ========== [Turbofan] Modify ARM vswp assembler test to use integers. Attempt to fix or get insight into failing vswp test on V8 ARM bot. LOG=N BUG= Committed: https://crrev.com/9e3feefff2fa2740515e38b50d0908588f0fb49e Cr-Commit-Position: refs/heads/master@{#41397} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9e3feefff2fa2740515e38b50d0908588f0fb49e Cr-Commit-Position: refs/heads/master@{#41397}
Message was sent while issue was closed.
Looks like this fixed things! Thanks! |
