Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2515803002:
[turbofan] Use correct block when tracing pending assessments in regalloc verifier (Closed)
|
Created:
4 years, 1 month ago by Mircea Trofin Modified:
4 years, 1 month ago Reviewers:
bradnelson CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Use correct block when tracing pending assessments in regalloc verifier
The verifier needs to use the block and assessments in that block corresponding to
a predecessor of a "pending" assessment. Not doing that causes incorrect
assessments when 2 locations are swapped.
BUG=665402
Committed: https://crrev.com/71144e5aa690eb6a964b155cb3a9d02a3603c968
Cr-Commit-Position: refs/heads/master@{#41159}
Patch Set 1 : testcase #Patch Set 2 : fix #
Messages
Total messages: 26 (17 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by mtrofin@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 ========== [turbofan] Fix for 665402 BUG= ========== to ========== [turbofan] Fix for 665402 BUG=665402 ==========
The CQ bit was checked by mtrofin@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 ========== [turbofan] Fix for 665402 BUG=665402 ========== to ========== [turbofan] Fix for 665402 Patch#1 is repro-ing the bug. Patch#2 demonstrates the fix. BUG=665402 ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you capture in the commit message what the issue was? As the wasm bytes aren't inspectable, it makes this tricky to follow. I wonder if we should consider DEPSing in a version of wabt and checking in wasts for these. Not for the change though.
Description was changed from ========== [turbofan] Fix for 665402 Patch#1 is repro-ing the bug. Patch#2 demonstrates the fix. BUG=665402 ========== to ========== [turbofan] Fix for 665402 The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes in correct assessments when 2 locations are swapped. BUG=665402 ==========
done. Indeed, ideally we'd check in wabt or mjsunit Wasm Builder instructions. I tried the latter using wasm::PrintAstForDebugging, however, it seems the js builder and the decoder are out of sync.
Description was changed from ========== [turbofan] Fix for 665402 The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes in correct assessments when 2 locations are swapped. BUG=665402 ========== to ========== [turbofan] Use correct block when tracing pending assessments in regalloc verifier The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes in correct assessments when 2 locations are swapped. BUG=665402 ==========
Fixed commit header
ping :)
Description was changed from ========== [turbofan] Use correct block when tracing pending assessments in regalloc verifier The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes in correct assessments when 2 locations are swapped. BUG=665402 ========== to ========== [turbofan] Use correct block when tracing pending assessments in regalloc verifier The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes incorrect assessments when 2 locations are swapped. BUG=665402 ==========
The CQ bit was checked by bradnelson@chromium.org
lgtm
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": 1479765305640450,
"parent_rev": "1ecbefb44d195f2a0abaeef7420e4e4324c61ef0", "commit_rev":
"d886d8c9b823c877d6c4b8537218beea88907e1e"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Use correct block when tracing pending assessments in regalloc verifier The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes incorrect assessments when 2 locations are swapped. BUG=665402 ========== to ========== [turbofan] Use correct block when tracing pending assessments in regalloc verifier The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes incorrect assessments when 2 locations are swapped. BUG=665402 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Use correct block when tracing pending assessments in regalloc verifier The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes incorrect assessments when 2 locations are swapped. BUG=665402 ========== to ========== [turbofan] Use correct block when tracing pending assessments in regalloc verifier The verifier needs to use the block and assessments in that block corresponding to a predecessor of a "pending" assessment. Not doing that causes incorrect assessments when 2 locations are swapped. BUG=665402 Committed: https://crrev.com/71144e5aa690eb6a964b155cb3a9d02a3603c968 Cr-Commit-Position: refs/heads/master@{#41159} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/71144e5aa690eb6a964b155cb3a9d02a3603c968 Cr-Commit-Position: refs/heads/master@{#41159} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
