Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2008493002:
[x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads int… (Closed)
Description[x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches.
Sequences like:
1: Load[kRepWord32|kTypeInt32](<address>, ...)
2: Word32And(1, <constant>)
3: Word32Equal(2, <another constant>)
4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>)
5: Branch[None](3, ...) -> B1, B2
where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive.
What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing:
mov [address], <value>
test [address], <another constant>
With this patch, it becomes:
mov reg, [address]
mov [address], <value>
test reg, <another constant>
BUG=chromium:611976
Committed: https://crrev.com/0d22e7e46a73bebb1e92768a8443a7b7b101051b
Cr-Commit-Position: refs/heads/master@{#36482}
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : Update. #
Messages
Total messages: 27 (14 generated)
epertoso@chromium.org changed reviewers: + jarin@chromium.org
Please take a look. I'm not sure this is the right way to solve the issue.
The CQ bit was checked by epertoso@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008493002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008493002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/7757) v8_win64_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
Description was changed from ========== [x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches. Sequences like: 1: Load[kRepWord32|kTypeInt32](<address>, ...) 2: Word32And(1, <constant>) 3: Word32Equal(2, <another constant>) 4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>) 5: Branch[None](3, ...) -> B1, B2 where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive. What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing: mov [address], <value> test [address], <another constant> With this patch, it becomes: mov reg, [address] mov [address], <value> test reg, <another constant> BUG=611976 ========== to ========== [x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches. Sequences like: 1: Load[kRepWord32|kTypeInt32](<address>, ...) 2: Word32And(1, <constant>) 3: Word32Equal(2, <another constant>) 4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>) 5: Branch[None](3, ...) -> B1, B2 where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive. What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing: mov [address], <value> test [address], <another constant> With this patch, it becomes: mov reg, [address] mov [address], <value> test reg, <another constant> BUG=611976 ==========
jarin@chromium.org changed reviewers: + bmeurer@chromium.org
The CQ bit was checked by epertoso@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008493002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008493002/20001
Description was changed from ========== [x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches. Sequences like: 1: Load[kRepWord32|kTypeInt32](<address>, ...) 2: Word32And(1, <constant>) 3: Word32Equal(2, <another constant>) 4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>) 5: Branch[None](3, ...) -> B1, B2 where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive. What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing: mov [address], <value> test [address], <another constant> With this patch, it becomes: mov reg, [address] mov [address], <value> test reg, <another constant> BUG=611976 ========== to ========== [x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches. Sequences like: 1: Load[kRepWord32|kTypeInt32](<address>, ...) 2: Word32And(1, <constant>) 3: Word32Equal(2, <another constant>) 4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>) 5: Branch[None](3, ...) -> B1, B2 where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive. What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing: mov [address], <value> test [address], <another constant> With this patch, it becomes: mov reg, [address] mov [address], <value> test reg, <another constant> BUG=chromium:611976 ==========
lgtm
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 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/2008493002/#ps40001 (title: "Update.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008493002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008493002/40001
Message was sent while issue was closed.
Description was changed from ========== [x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches. Sequences like: 1: Load[kRepWord32|kTypeInt32](<address>, ...) 2: Word32And(1, <constant>) 3: Word32Equal(2, <another constant>) 4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>) 5: Branch[None](3, ...) -> B1, B2 where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive. What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing: mov [address], <value> test [address], <another constant> With this patch, it becomes: mov reg, [address] mov [address], <value> test reg, <another constant> BUG=chromium:611976 ========== to ========== [x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches. Sequences like: 1: Load[kRepWord32|kTypeInt32](<address>, ...) 2: Word32And(1, <constant>) 3: Word32Equal(2, <another constant>) 4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>) 5: Branch[None](3, ...) -> B1, B2 where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive. What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing: mov [address], <value> test [address], <another constant> With this patch, it becomes: mov reg, [address] mov [address], <value> test reg, <another constant> BUG=chromium:611976 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches. Sequences like: 1: Load[kRepWord32|kTypeInt32](<address>, ...) 2: Word32And(1, <constant>) 3: Word32Equal(2, <another constant>) 4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>) 5: Branch[None](3, ...) -> B1, B2 where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive. What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing: mov [address], <value> test [address], <another constant> With this patch, it becomes: mov reg, [address] mov [address], <value> test reg, <another constant> BUG=chromium:611976 ========== to ========== [x64/ia32] Deal with the non-transitivity of InstructionSelector::CanCover() when folding loads into branches. Sequences like: 1: Load[kRepWord32|kTypeInt32](<address>, ...) 2: Word32And(1, <constant>) 3: Word32Equal(2, <another constant>) 4: Store[(kRepWord32 : NoWriteBarrier)](<address>, <value>) 5: Branch[None](3, ...) -> B1, B2 where #1 and #4 refer to the same memory location, are problematic because in VisitBranch we assume that 'InstructionSelector::CanCover()' is transitive. What happens is that CanCover(5, 3) is true (3 is a pure op), and so are CanCover(3, 2), CanCover(2, 1), but the effect level of 5 and 3 never gets checked because 3 is a pure op. Upon VisitBranch, we ended up materializing: mov [address], <value> test [address], <another constant> With this patch, it becomes: mov reg, [address] mov [address], <value> test reg, <another constant> BUG=chromium:611976 Committed: https://crrev.com/0d22e7e46a73bebb1e92768a8443a7b7b101051b Cr-Commit-Position: refs/heads/master@{#36482} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0d22e7e46a73bebb1e92768a8443a7b7b101051b Cr-Commit-Position: refs/heads/master@{#36482}
Message was sent while issue was closed.
peter.verswyvelen@smartphoto.com changed reviewers: + peter.verswyvelen@smartphoto.com
Message was sent while issue was closed.
It seems my customer (www.smartphoto.com) is affected by this bug, our HTML5 photo product editor became unpredictable in Chrome, some functions when optimized behave incorrectly. Disabling the optimization by wrapping the function body in a try block solves the problem. Canary (version 53.0.2760.0) works fine. Do you have any idea when this patch will be released?
Message was sent while issue was closed.
epertoso@chromium.org changed reviewers: + hablich@chromium.org
Message was sent while issue was closed.
hablich@: do we have an ETA on this?
Message was sent while issue was closed.
On 2016/06/07 10:25:21, epertoso wrote: > hablich@: do we have an ETA on this? I was wrong, it seems we got bitten by another V8 bug, https://bugs.chromium.org/p/chromium/issues/detail?id=604033
Message was sent while issue was closed.
On 2016/06/07 10:25:21, epertoso wrote: > hablich@: do we have an ETA on this? I was wrong, it seems we got bitten by another V8 bug, https://bugs.chromium.org/p/chromium/issues/detail?id=604033 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
