Page MenuHomePhabricator

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

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


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

silvas added a subscriber: silvas.Jul 8 2021, 10:29 AM
silvas added inline comments.

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.

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.

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

Oh, fascinating! TIL!