[PATCH] D74342: [mlir] Add MemRef filter to affine data copy optimization

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/include/mlir/Transforms/LoopUtils.h:228
+void gatherLoops(Block *block, unsigned currLoopDepth,
+                 DenseMap<unsigned, SmallVector<AffineForOp, 2>> &depthToLoops);
+
----------------
Seems like a private implementation for the one below, do we need to expose it publicly?
The API is leaking the fact that this is recursion helper...


================
Comment at: mlir/lib/Transforms/Utils/LoopUtils.cpp:1796-1797
+    }
+  }
+}
+
----------------
DenseMap is a surprising choice to me for a container that will be filled with monotonic indices starting at zero...
Can this be just a SmallVectorImpl<SmallVector<AffineForOp, 2>> ?

I think you just need to add this:
  if (currLoopDepth >= depthToLoops.size()) depthToLoops.resize(currLoopDepth);

at the beginning of the function and change nothing else.



================
Comment at: mlir/test/lib/Transforms/TestAffineDataCopy.cpp:29
+        "Enable memref filter testing in affine data copy optimization"),
+    llvm::cl::cat(clOptionsCategory));
+
----------------
Just a nit: can you use a pass option. Not that it matters much for a test pass but it'll display more consistently for `mlir-opt --help`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74342