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

[rsuderman]

Summary

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

[mravishankar]

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.

[rsuderman]

Updated to add tensor reshape canonicalization test.

[rsuderman]

In D102282#2752291, @mravishankar wrote:

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]

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.

[mravishankar]

mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp

1140–1141

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

[mravishankar]

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

mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp

1140–1141

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

[rsuderman]

Fixed for second round of review

[nicolasvasilache]

This one slipped by me a long time ago .. I am concerned about the semantics implications of this change.
For valid collapse / expand ops, the invariant has been that the op needs to list all dimensions in order, from 0 to the largest rank.
It seems the verifier for this is not strict enough.
I see the code is quite old and still based on AffineMap.

I'd like to clean all that old tech debt up and just indices.
Could you please elaborate on what kind of transform needed to introduce such IR ?

mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp

while (sourceDim < sourceShape.size()) {
     // should match the collapsed dimension shape.
     if (prodOfCollapsedDims * sourceShape[sourceDim] != currTargetShape)
       return llvm::None;
 
     currIndices.push_back(sourceDim++);
     // If there are no dimensions in the target to match, then append the
     // `currIndices` to the last element of the reassociationMap.
     if (targetDim == targetShape.size()) {
+      if (!reassociationMap.empty())
         reassociationMap.back().append(currIndices.begin(), currIndices.end());
       // Break out of the loops. We should be done here.
       break;
     }
     reassociationMap.emplace_back(ReassociationIndices{});
     std::swap(reassociationMap.back(), currIndices);
     prodOfCollapsedDims = 1;
   }
   // All the dimensions in the two shapes must have been processed.