[Mlir-commits] [mlir] [MLIR][Tensor] Remove FoldDimOf[Expand|Collapse]Shape Pattern (PR #134219)

[Kunwar Grover]

Groverkss wrote:

Ok let's take a step back and see why this patch is a good patch. I looked through the history and code and here is the reasoning:

There are two questions we are answering here:

1. Is the current implementation of this pattern wrong? Should we fix it?
2. Should the correct pattern be a canonicalization?

The answer to Question 1, is that this pattern is wrong. It's trying to infer the output shape of the tensor, which is already provided in the operation! This pattern is from the time when expand_shape did not carry this information. The output shape was explicitly added to the operation in https://github.com/llvm/llvm-project/pull/90040 and has reasoning why for dynamic shapes, the output shape inference is wrong.

So now we have two options, either fix the pattern, or remove it. Thing is, the same pattern is implemented correctly through ReifyRankedShapedTypeOpInterface as a general pattern. So it is okay to remove the pattern, as once fixed, this pattern is a duplicate of a more general pattern.

So now, to Question 2, is the correct pattern, in whatever shape or form, a good canonicalization? For static cases, expand_shape already has a folder to move to a more canonical form, where the `output_shape` field in the op moves to a more canonical form (dynamic -> static). For dynamic cases, the answer to this question is No.  The pattern is not a canonicalization for expand_shape at all! In it's correct form, it doesn't change the representation of expand_shape in any form, instead it is using information from expand_shape to bubble up the tensor.dim operations upwards. What I understand by "non-local" effects here is that it's doing transformations around the operation that is meant to be canonicalized, but not doing anything to the operation itself. So it's "non-local" to the operation being canonicalized. This is a clear indication that this pattern is not really a canonicalization. 

Finally, The pattern is implemented in transformations that refine shape information through ReifyRankedShapedTypeOpInterface so we aren't losing any old transformations here, this patch is fixing the questions I explained above.

@joker-eph @MaheshRavishankar Hope this clears it up. I think this should provide a clear answer why this pattern is being removed as a canonicalization, and also being deleted at the same time. Let me know if you need more clarification.

https://github.com/llvm/llvm-project/pull/134219