This addresses the issue reported in
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
I am not sure this is a good solution. Requiring from OffsetSizeAndStrideOpInterface users to to do such index manipulation is very inconvenient and error-prone. Maybe better to separate isDynamicDim APIs into 2 sets isInputDynamicDim/isOutputDynamicDim which can do index translation under the hood?
Also, locally I workarounded this by never using rank-reducing subview and instead implementing a custom op to do a rank reduction on memrefs (with this approach I can also precisely control, which dimensions I want to collapse, not just all-or-nothing).
see comment
otherwise LGTM, thanks
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | ||
---|---|---|
274 | Is this part of this patch? It needs tests then |
This simple implementation is unfortunately not extensible and needs to be reverted.
The extensible way should be to extend https://reviews.llvm.org/D104321.
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | ||
---|---|---|
277 | btw, I don't think folders are allowed to mutate the op like this. This needs to be a canonicalization pattern. |
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | ||
---|---|---|
277 | I think this is fine, see the MLIR doc: "fold has the restriction that no new operations may be created, and only the root operation may be replaced. It allows for updating an operation in-place, or returning a set of pre-existing values (or attributes) to replace the operation with" |
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | ||
---|---|---|
277 | Folders can mutate the op in place - in fact, the folder is prefered to a canonicalization pattern for such local op mutation because it lets the rewrite driver converge faster (to do more work early) -- since the folder would have run on it before the highest benefit canonicalization pattern is picked later in an iteration of the rewriter driver. |
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp | ||
---|---|---|
277 | Oh, fascinating! TIL! |
Is this part of this patch? It needs tests then