[wasm][revec] Avoid changing default node group [v8/v8 : main]
Chen, Yolanda (Gerrit)
Chen, Yolanda added 1 comment![]( "Open in Gerrit")
Hi Matthias and Jakob, this fixed a newly reported bug by CL: https://chromium-review.googlesource.com/c/v8/v8/+/7299683 . PTAL, Thanks!
Related details
- Jakob Kummerow
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke added 1 comment![]( "Open in Gerrit")
if (offset0 != offset1) {Is it expected that we never use `offset1` here?
Related details
- Chen, Yolanda
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chen, Yolanda (Gerrit)
Chen, Yolanda added 1 comment![]( "Open in Gerrit")
if (offset0 != offset1) {Is it expected that we never use `offset1` here?
I think offset1 is already reduced here and maybe used by other nodes. Here we emit offset0 just to make it valid when emit the Simd256LoadTransform node.
Related details
- Jakob Kummerow
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chen, Yolanda (Gerrit)
Chen, Yolanda added 1 comment![]( "Open in Gerrit")
if (offset0 != offset1) {Chen, YolandaIs it expected that we never use `offset1` here?
I think offset1 is already reduced here and maybe used by other nodes. Here we emit offset0 just to make it valid when emit the Simd256LoadTransform node.
We can also skip this check, just it will generate a duplicate offset constant.
Related details
- Jakob Kummerow
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
1 comment
if (offset0 != offset1) {Chen, YolandaIs it expected that we never use `offset1` here?
Chen, YolandaI think offset1 is already reduced here and maybe used by other nodes. Here we emit offset0 just to make it valid when emit the Simd256LoadTransform node.
We can also skip this check, just it will generate a duplicate offset constant.
Ah, makes sense, fine for me to skip the duplication here, though GVN should handle this as well.
Related details
- Chen, Yolanda
- Jakob Kummerow
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow added 1 comment![]( "Open in Gerrit")
if (offset0 != offset1) {Chen, YolandaIs it expected that we never use `offset1` here?
Chen, YolandaI think offset1 is already reduced here and maybe used by other nodes. Here we emit offset0 just to make it valid when emit the Simd256LoadTransform node.
Matthias LiedtkeWe can also skip this check, just it will generate a duplicate offset constant.
Ah, makes sense, fine for me to skip the duplication here, though GVN should handle this as well.
I think this condition is always true: `offset0` comes from the `__ input_graph()`, `offset1` comes from the `__ output_graph()`, so they can't ever be the same node. So unless I'm missing something, I think the change in this file is actually a no-op? If so, it'd be simpler to revert it.
Related details
- Chen, Yolanda
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
1 comment
if (offset0 != offset1) {Chen, YolandaIs it expected that we never use `offset1` here?
Chen, YolandaI think offset1 is already reduced here and maybe used by other nodes. Here we emit offset0 just to make it valid when emit the Simd256LoadTransform node.
Matthias LiedtkeWe can also skip this check, just it will generate a duplicate offset constant.
Jakob KummerowAh, makes sense, fine for me to skip the duplication here, though GVN should handle this as well.
I think this condition is always true: `offset0` comes from the `__ input_graph()`, `offset1` comes from the `__ output_graph()`, so they can't ever be the same node. So unless I'm missing something, I think the change in this file is actually a no-op? If so, it'd be simpler to revert it.
Never mind, I was confused.
Related details
- Chen, Yolanda
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chen, Yolanda (Gerrit)
Chen, Yolanda voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[wasm][revec] Avoid changing default node group
Changing default node group caused missing node mapping. Instead we can
check the offsets for duplicate LoadSplat when reduce the nodes.
- M src/compiler/turboshaft/wasm-revec-reducer.cc
- M src/compiler/turboshaft/wasm-revec-reducer.h
- M test/cctest/wasm/test-run-wasm-simd.cc
Code-Review: +1 by Jakob Kummerow, +1 by Matthias Liedtke
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |