This is an archive of the discontinued LLVM Phabricator instance.

[mlir][MemRef] Fix DimOp folding of OffsetSizeAndStrideInterface.
ClosedPublic

Authored by nicolasvasilache on Jul 7 2021, 9:11 AM.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Jul 7 2021, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 9:11 AM

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).

Add a ShapedType::getRelativeIndexOfDynamicDim helper.

Make index actually start at 0.

Hardcode84 accepted this revision.Jul 7 2021, 5:00 PM

see comment

otherwise LGTM, thanks

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
274

Is this part of this patch? It needs tests then

This revision is now accepted and ready to land.Jul 7 2021, 5:00 PM
This revision was landed with ongoing or failed builds.Jul 8 2021, 1:34 AM
This revision was automatically updated to reflect the committed changes.

This simple implementation is unfortunately not extensible and needs to be reverted.
The extensible way should be to extend https://reviews.llvm.org/D104321.

silvas added a subscriber: silvas.Jul 8 2021, 10:29 AM
silvas added inline comments.
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.

nicolasvasilache marked an inline comment as done.Jul 8 2021, 3:40 PM
nicolasvasilache added inline comments.
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"

bondhugula added inline comments.
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.

silvas added inline comments.Jul 9 2021, 10:47 AM
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
277

Oh, fascinating! TIL!