[PATCH] D72935: [mlir] Add a canonicalization pattern for MemRefCastOp into dynamic MemRefs

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 3 inline comments as done.
nicolasvasilache added inline comments.


================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:1768
+                                     PatternRewriter &rewriter) const override {
+    MemRefType sourceType = castOp.source().getType().cast<MemRefType>();
+    AffineMap sourceMap = (sourceType.getAffineMaps().empty())
----------------
nmostafa wrote:
> You need to handle or bailout on UnrankedMemRefType (and probably add a test for it). Also, the type propagation is only valid based on how the result is used : e.g. if a func argument, then the conversion is invalid. 
Thanks, I addressed those points but there are quite deeper implications as River pointed out.



================
Comment at: mlir/lib/Dialect/StandardOps/Ops.cpp:1804
+
+    rewriter.replaceOpWithNewOp<MemRefCastOp>(castOp, castOp.source(),
+                                              desiredResultType);
----------------
rriddle wrote:
> You are replacing with something of a different type. That doesn't really fit as a canonicalization. For this to be conservatively correct, it would mean you need to prevent a memref cast from ever being used: in a function call, as a branch argument, etc.
How fundamental do you foresee this to be a limitation in the longer term?
The use cases look like: 
```
  cast -> X1 
  ...
  cast -> Xk 
```
where k is small(ish). 
Very concretely, this enables n-D linag.vectorization to kick-in as a declarative pattern rewrite that composes nicely with all the other things.
So at some point I will want a way to hook that up with Ops that should not be seen here.

I could extend this into a conditional pattern that is called with a lambda from the proper places.
It also seems OpInterfaces would be a natural fit but it seems a very unnatural thing to say "set of ops for which memrefcast can canonicalize its return type".

OTOH I can also bail here and make this a rewrite pattern locally in the pass that I am experimenting with.
But the way things start looking like passes become a union of loose connected patterns that compose nicely and locally to achieve global effects.
The most natural thing that this pattern made me think of is a canonicalization (modulo the type change), which is why I am trying this maybe unnatural thing.

Thoughts?
 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72935/new/

https://reviews.llvm.org/D72935