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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Sorry, I've been OOO but it looks like Alex was able to give an amazing run down. Thanks Alex!



================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.h:13
+
+#ifndef MLIR_INTERFACES_AFFINEMEMORYOPINTERFACES_H_
+#define MLIR_INTERFACES_AFFINEMEMORYOPINTERFACES_H_
----------------
nit: This is wrong.


================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td:48
+    InterfaceMethod<
+      /*desc=*/[{ Returns affine map operands. }],
+      /*retTy=*/"Operation::operand_range",
----------------
nit: You can use a normal string literal for one line descriptions: "..."


================
Comment at: mlir/include/mlir/Dialect/Affine/IR/AffineMemoryOpInterfaces.td:59
+    InterfaceMethod<
+      /*desc=*/[{ Returns the affine map used to index the memref for this
+                  operation. }],
----------------
nit: These are generally formatted as:

... [{
  ...
}]


================
Comment at: mlir/include/mlir/Dialect/Affine/IR/CMakeLists.txt:4
+
+set(LLVM_TARGET_DEFINITIONS AffineMemoryOpInterfaces.td)
+mlir_tablegen(AffineMemoryOpInterfaces.h.inc -gen-op-interface-decls)
----------------
Seems like this should be using add_mlir_interface?


================
Comment at: mlir/lib/Analysis/AffineAnalysis.cpp:666
+  } else {
+    auto storeOp = cast<AffineWriteOpInterface>(opInst);
     map = storeOp.getAffineMap();
----------------
nit: I would have just inlined the use of the storeOp


================
Comment at: mlir/lib/Dialect/Affine/IR/AffineMemoryOpInterfaces.cpp:1
+//===- AffineMemoryOpInterfaces.cpp - Loop-like operations in MLIR --------===//
+//
----------------
Loop-Like?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79829