[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/lib/Transforms/Utils/LoopUtils.cpp:1796-1797
+    }
+  }
+}
+
----------------
dcaballe wrote:
> mehdi_amini wrote:
> > 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.
> > 
> I forgot to ask the same question to @andydavis1! I thought that maybe there was a use case that I wasn't taking into account. Even if we wanted to update the container after a loop transformation, we would be adding or removing loop levels to/from the back. ?
> 
> Another comment about the current implementation is that it adds an extra "empty" level. When we process the innermost loop and we do:
> ```
> auto &loopsAtDepth = depthToLoops[currLoopDepth];
> ``` 
> we create a new level that won't contain any loop.
> 
> I'll take care of both things if it makes sense to you.
> 
> > if (currLoopDepth >= depthToLoops.size()) depthToLoops.resize(currLoopDepth);
> Not sure I follow. Wouldn't this condition be always false and cause a realloc every time we add a new level?
> Not sure I follow. Wouldn't this condition be always false and cause a realloc every time we add a new level?

If this is a private helper yes you can skip the check.
If it is a public function you would resize by truncating if the depth was smaller than the vector size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74342