@thib...@chromium.org: Can I offload this change to reduce my review-load? I was out for a few days and I have a bunch of reviews in my pipeline.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
Sorry for the wait, this is a pretty large change and I had to look at the original change that introduces I32x4DotI8x16S to understand the context better.
LGTM with a nit and some questions for my own understanding.
// Represents one candidate dot-product reduction rooted at an
// I32x4AddReduce. The search walks backwards through a tree of I32x4Add,
// Phi, and I32x4DotI8x16S operations, collecting dots whose generated input
// shuffles can be removed.Could we explain the motivation behind this optimization here? I.e. that this is to undo the shuffle that we introduce in https://crrev.com/c/7956960.
There is such a comment at wasm-shuffle-reducer.cc:292 but I think it deserves to be more visible.
// Sub Dot / |In principle couldn't we also visit the inputs of Sub?
The elements of the input will either all be subtracted or all be added to the total, so shuffling them shouldn't affect the result.
// We allow any other I32x4AddReduce operation that we didn't discover as
// part of the search.
if (const Simd128ReduceOp* reduce = use_op.TryCast<Simd128ReduceOp>()) {
if (reduce->kind == Simd128ReduceOp::Kind::kI32x4AddReduce) continue;
}
TRACE("Reduction node %d has outside user %d\n", node.id(), use.id());
return false;So if a Dot belongs to multiple trees, something like this:
```
A Dot B
\ / \ /
Add Add
| |
AddReduce AddReduce
```
The shuffle could be eliminated in principle, but we will not detect it in this case because the Dot does not flow directly into the other AddReduce, right?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |