This is an archive of the discontinued LLVM Phabricator instance.

[mlir][memref] Fix CollapseShapeOp verifier
ClosedPublic

Authored by springerm on Mar 29 2022, 5:18 AM.

Diff Detail

Event Timeline

springerm created this revision.Mar 29 2022, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 5:18 AM
springerm requested review of this revision.Mar 29 2022, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 5:18 AM
mravishankar requested changes to this revision.Mar 29 2022, 12:37 PM
mravishankar added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
1790

Calling out the same assumption as the expand shape patch. The strides might not be in the same order as the dimensions. So if this is "unsupported" behavior needs to be verified or called out in the op semantics.

1864

Can we return an actual error message here?

This revision now requires changes to proceed.Mar 29 2022, 12:37 PM
nicolasvasilache requested changes to this revision.Mar 30 2022, 12:17 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
1807

Same comments as in https://reviews.llvm.org/D122641, with likely a slightly different implementation solving the problem with much lower code complexity.

1883

same here re https://reviews.llvm.org/D122641, use of (generalized) saturated_arith will reduce all this complexity and will turn into a simple reduce 1-liner idiom (or map idiom with capture if we don't have a reduce idiom).

springerm updated this revision to Diff 419067.Mar 30 2022, 1:21 AM

address comments

springerm updated this revision to Diff 419070.Mar 30 2022, 1:38 AM

simplify code

simplify code

mravishankar accepted this revision.Mar 30 2022, 12:40 PM

If I am reading this right, this works for the case where stride are not row-major ordered. If so please add a test for that. Removing my blocker

This revision was not accepted when it landed; it landed in state Needs Review.Mar 31 2022, 1:10 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

I am unclear why this landed as there were outstanding unaddressed comments.
Sent a followup cleanup here: https://reviews.llvm.org/D122845

Oh I must have landed this by accident.