[PATCH] D92974: [OpenMPIRBuilder] Implement tileLoops.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

I'm fine with the `std::vector`. I'd prefer to avoid complex cleanup but I'm not insisting. I left one comment to address below, other than that LGTM.

@ftynse Would be great if you approve as soon as you're OK with this.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:302
+  ///
+  /// Tiles the loops of \p CL by the tile sizes in \p Sizes. Loops in \p Loops
+  /// must be perfectly nested, from outermost to innermost loop (i.e.
----------------
ftynse wrote:
> Nit: I don't see `CL` and `Sizes` in the argument list
It's `TileSizes` now.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:323
+  /// \endcode
+  ///
+  /// The returned vector are the loops {i1,j1,i2,j2}. The loops i1 and j1 are
----------------
ftynse wrote:
> What happens when tile sizes don't divide loop sizes perfectly?
I think that case is handled by the code and the comment below states it too.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1310
+    return EmbeddedLoop;
+  };
+
----------------
Leftover commented code and please add a short description here and below.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1205
+  DeleteDeadBlocks(BBVec);
+}
+
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Even if we make blocks dead, do we really want to clean up here?
> The Verifier complains about (1.) orphaned PHI nodes, (2.) def-use dominance and (3.) degenerate blocks, even in unreachable BBs . Removing all unused blocks seemed to be the cleanest solution.
> 
> Quickly running unittest `removeUnusedBlocksFromParent` currently does not fail. I guess 1. was resolved by Succ->removePredecessor` in `redirectTo` (which I needed to add because there were other situations that required will-formed PHI nodes), 2. already has an exception in `DominatorTree::dominates` and 3. currently no BBs without terminators are left behind.
> 
> If you don't mind the occasional orphaned BB, we might remove the entire `removeUnusedBlocksFromParent`.
I would not mind generating some unused blocks. The algorithm we use here to delete them seems a bit much given that we will basically remove them for free later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92974