This revision adds 0-d vector support to vector.transfer ops.
In the process, numerous cleanups are applied, in particular around normalizing
and reducing the number of builders.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/test/Dialect/Vector/ops.mlir | ||
---|---|---|
7–14 | Can 0-D vector be read from non 0-D tensor? It would be good to add a test for it. |
Address comment.
Use mandatory empty AffineMapAttr instead of optional null to reduce corner cases as suggested offline by @ThomasRaoux.
Thanks for helping with the 0-d vector problem! Added some minor comments. I have a question about the lowering of 0-d vectors to the scalar world. See comments inline.
mlir/include/mlir/Dialect/Vector/VectorOps.td | ||
---|---|---|
1157 | +1, same for xfer write | |
1306–1329 | and an? | |
mlir/lib/Conversion/VectorToROCDL/VectorToROCDL.cpp | ||
69 | These checks on GPU and ROCDL mean that 0-d tensors are not expected at all by these backends or it's more a TODO to be implemented in the future? If the latter, we should add a TODO comment staying that it will be implemented in the future. | |
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp | ||
230 | This looks like we are vectorizing the operand to be stored in place. Shouldn't this call to some generic function that takes care of that (vectorizeOneOp or some other one)? | |
mlir/lib/Dialect/Vector/VectorOps.cpp | ||
2317 | and an? | |
mlir/lib/Dialect/Vector/VectorTransforms.cpp | ||
2805 | Move comment before the if? | |
2870–2877 | load? | |
2895 | Question about this lowering: why are we lowering 0-d vectors to the scalar world? I thought one of the goals discussed in the RFC was to keep everything within the vector world to avoid the scalar<->vector transition that could be very expensive for some targets. Shouldn't we lower 0-d vectors to a `vector<1xtype> in LLVM instead? |
mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp | ||
---|---|---|
74–76 | we check that the rank must be 2 right below so this won't really change anything. I'm assuming you are just adding it from consistency? (same below) |
mlir/include/mlir/Dialect/Vector/VectorOps.td | ||
---|---|---|
1157 | Because the comment was off by 1.
empty as expected; before it used to be 2 .. 1 which is weird
| |
1329 | dead now | |
1464 | dead | |
mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp | ||
74–76 | Yes the evolution has been that I broke the API to make sure by way of compiler crashes that I am not missing any entry. Then I just copied this everywhere. In this very special case I can just remove indeed. | |
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp | ||
230 | This is a side-effect of the piece below that says: // Not all ops support 0-d vectors, extract the scalar for now. // TODO: remove this. if (readValue.getType().cast<VectorType>().getRank() == 0) readValue = b.create<vector::ExtractElementOp>(loc, readValue); Both should go away once all ops support 0-d vectors. | |
mlir/lib/Dialect/Vector/VectorTransforms.cpp | ||
2895 | Note that this is not changing any behavior here and we were already doing this extraction for vector<1x..x1xtype>.
There are still deeper data layout issues lingering even in this trivial case (for architectures for which this matters that is .. ). |
why did this change?