[PATCH] D79829: [mlir][Affine] Introduce affine memory interfaces

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

Thanks for the feedback!

> I am wondering whether AffineMemoryOpInterfaces belongs in lib/Interfaces since it is specific to the Affine dialect. I'd rather put it together with the dialect, even though the transformations live in lib/Transforms

Good point. That makes sense. Done.

> For the names AffineReadLikeOpInterface, AffineWriteLikeOpInterface, we could even consider dropping "Like" from it. It has become standard to use "Like" in interfaces because the prefixes are often the names of the ops. But here, "read" and "write" are already capturing the "like" part unlike say LoopLikeOp where you have a LoopOp and so it has to be LoopLikeOp. So, if you/others are fine with it, you could go for AffineReadOpInterface, AffineWriteOpInterface. DMAs, load/stores and vector load/stores - all read/write data.

Agreed! Done.



================
Comment at: mlir/include/mlir/Dialect/Affine/IR/CMakeLists.txt:2
 add_mlir_dialect(AffineOps affine)
+#TODO: Add MLIRAffineMemoryOpInterfacesIncGen dependency
 add_mlir_doc(AffineOps -gen-op-doc AffineOps Dialects/)
----------------
ftynse wrote:
> Please do :)
Sorry, I wanted to ask about it since I didn't see any other tablegen->tablegen dependencies like this one so I'm not sure how to do it. For example, we should also have a dependency with the loop-like interface here, right?

I added a dependency between both targets but this is not enough. I think we need to add a dependency between the dialect target and the interface .td file. Not sure how to do that cleanly.


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineOps.cpp:2564-2581
+/// Get memref operand.
+Value AffineVectorLoadOp::getMemRef() {
+  return getOperand(getMemRefOperandIndex());
+}
+
+MemRefType AffineVectorLoadOp::getMemRefType() {
+  return getMemRef().getType().cast<MemRefType>();
----------------
bondhugula wrote:
> I missed why we need to introduce these here? Shouldn't they be shared via the op interface method?
Are you suggesting using the interface also as a base class? I assumed that interfaces shouldn't have any implementation. That could work for some methods but not all of them have the same implementation (e.g., `getMapOperands`). I could move those with the same implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79829