Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 10 months ago by epertoso Modified:
4 years, 10 months ago Reviewers:
*Benedikt Meurer CC:
Camillo Bruni, v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Fix a bug in the RawMachineAssembler
This was causing code like:
REX.W cmpq r9,r8
setzl r8l
movzxbl r8,r8
REX.W cmpq r8,0x0
jz 185
(note the cmpq instead of cmpl above) on x64 instead of:
REX.W cmpq r9,r8
jnz 149
http://crrev.com/1677503002 is now obsolete and has been reverted.
Committed: https://crrev.com/61a4c528b705f36899b72b02fb9b959b70494c00
Cr-Commit-Position: refs/heads/master@{#33934}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update. #Patch Set 3 : Fixes one ARM unit test. #
Created: 4 years, 10 months ago
Messages
Total messages: 20 (11 generated)
Patchset #1 (id:1) has been deleted
epertoso@chromium.org changed reviewers: + bmeurer@chromium.org
epertoso@chromium.org changed required reviewers: + bmeurer@chromium.org
Description was changed from ========== Fix a bug in the x64 instruction selector introduced in https://codereview.chromium.org/1677503002. While visiting a branch, a 64-bit word comparison between a Phi node and 0 yielded a kX64Cmp32 instruction. BUG= ========== to ========== [turbofan] Fix a bug in the x64 instruction selector introduced in https://crrev.com/1677503002. While visiting a branch, a 64-bit word comparison between a Phi node and 0 yielded a kX64Cmp32 instruction. ==========
https://codereview.chromium.org/1685183003/diff/20001/src/compiler/x64/instru... File src/compiler/x64/instruction-selector-x64.cc (right): https://codereview.chromium.org/1685183003/diff/20001/src/compiler/x64/instru... src/compiler/x64/instruction-selector-x64.cc:1457: case IrOpcode::kLoad: This looks rather hacky to me, esp. hunting for Load/Phi and special casing those. Only Word32Equal with 0 is ever used to negate a condition (actually maybe we should really consider introducing BitEqual to avoid the confusion), so performing this optimization for Word64Equal will probably not help anyway.
Patchset #2 (id:40001) has been deleted
PTAL https://codereview.chromium.org/1685183003/diff/20001/src/compiler/x64/instru... File src/compiler/x64/instruction-selector-x64.cc (right): https://codereview.chromium.org/1685183003/diff/20001/src/compiler/x64/instru... src/compiler/x64/instruction-selector-x64.cc:1457: case IrOpcode::kLoad: On 2016/02/11 at 05:13:53, Benedikt Meurer wrote: > This looks rather hacky to me, esp. hunting for Load/Phi and special casing those. Only Word32Equal with 0 is ever used to negate a condition (actually maybe we should really consider introducing BitEqual to avoid the confusion), so performing this optimization for Word64Equal will probably not help anyway. Yeah, I traced back the issue to the use of Word64BinaryNot. I've removed it from the RawMachineAssembler.
Description was changed from ========== [turbofan] Fix a bug in the x64 instruction selector introduced in https://crrev.com/1677503002. While visiting a branch, a 64-bit word comparison between a Phi node and 0 yielded a kX64Cmp32 instruction. ========== to ========== [turbofan] Fix a bug in the RawMachineAssembler This was causing code like: REX.W cmpq r9,r8 setzl r8l movzxbl r8,r8 REX.W cmpq r8,0x0 jz 185 (note the cmpq instead of cmpl above) on x64 instead of: REX.W cmpq r9,r8 jnz 149 http://crrev.com/1677503002 is now obsolete and has been reverted. ==========
Nice. LGTM
The CQ bit was checked by epertoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1685183003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685183003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/13647)
The CQ bit was checked by epertoso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/1685183003/#ps80001 (title: "Fixes one ARM unit test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1685183003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685183003/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Fix a bug in the RawMachineAssembler This was causing code like: REX.W cmpq r9,r8 setzl r8l movzxbl r8,r8 REX.W cmpq r8,0x0 jz 185 (note the cmpq instead of cmpl above) on x64 instead of: REX.W cmpq r9,r8 jnz 149 http://crrev.com/1677503002 is now obsolete and has been reverted. ========== to ========== [turbofan] Fix a bug in the RawMachineAssembler This was causing code like: REX.W cmpq r9,r8 setzl r8l movzxbl r8,r8 REX.W cmpq r8,0x0 jz 185 (note the cmpq instead of cmpl above) on x64 instead of: REX.W cmpq r9,r8 jnz 149 http://crrev.com/1677503002 is now obsolete and has been reverted. Committed: https://crrev.com/61a4c528b705f36899b72b02fb9b959b70494c00 Cr-Commit-Position: refs/heads/master@{#33934} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/61a4c528b705f36899b72b02fb9b959b70494c00 Cr-Commit-Position: refs/heads/master@{#33934} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
