Page MenuHomePhabricator

[mlir][linalg] Fixed issue generating reassociation map with Rank-0 types
ClosedPublic

Authored by rsuderman on May 11 2021, 3:32 PM.

Details

Summary

Rank-0 case causes a graph during linalg reshape operation.

Diff Detail

Event Timeline

rsuderman created this revision.May 11 2021, 3:32 PM
rsuderman requested review of this revision.May 11 2021, 3:32 PM
mravishankar requested changes to this revision.May 11 2021, 3:40 PM

Is it possible to get a small repro for the issue. Basically looking for the example where you hit this. I know why this might have happened, but want to make sure it is the expected case.

This revision now requires changes to proceed.May 11 2021, 3:40 PM
rsuderman updated this revision to Diff 344587.May 11 2021, 4:25 PM

Updated to add tensor reshape canonicalization test.

Is it possible to get a small repro for the issue. Basically looking for the example where you hit this. I know why this might have happened, but want to make sure it is the expected case.

Test added. It was a bit tricky as the case was only failing in intermittent IR during a pass.

mravishankar accepted this revision.May 11 2021, 5:18 PM

Thanks. Thats what I thought. It was for the case where unit-dimensions are all folded away. I wish there was a way to assert that in the code. (I think it is provable that this is always the case based on reshape semantics, but still having an assert that currIndices (in the code above) correspond to all unit-dimensions would be good.

This revision is now accepted and ready to land.May 11 2021, 5:18 PM
mravishankar added inline comments.May 11 2021, 5:18 PM
mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
1141–1142

Actually could you make this !reassociationMap.empty() && !currIndices.empty()

mravishankar requested changes to this revision.May 11 2021, 5:21 PM

Sorry, found a good way to check for the case.

mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
1141–1142

Also would be nice to leave a comment like

If the reassociation is empty but the currIndices is not, this by definition is folding unit-dimensions with the result being scalar type. So only append the `currIndices` if reassociation map is not empty.

THat gives me another idea. Could you return failure if the targetShape.size() != 0

This revision now requires changes to proceed.May 11 2021, 5:21 PM
rsuderman updated this revision to Diff 344610.May 11 2021, 5:25 PM

Fixed for second round of review

rsuderman marked 2 inline comments as done.May 11 2021, 5:25 PM
mravishankar accepted this revision.May 11 2021, 8:23 PM
This revision is now accepted and ready to land.May 11 2021, 8:23 PM