[PATCH] D92974: [OpenMPIRBuilder] Implement tileLoops.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:340
+  tileLoops(DebugLoc DL, ArrayRef<CanonicalLoopInfo *> Loops,
+            ArrayRef<Value *> TileSizes);
+
----------------
jdoerfert wrote:
> Any reason to use a std vector here? Would it make sense to extend Loops instead (after changing the type)? The original loop vector is still "contained" if we append the new ones.
The method returns new `CanonicalLoopInfo*` instances, the one passed by \p Loops are invalidated.

I don't like mixing in- and out- parameters that have different meaning just because it might fit the signature. Think of `void labs(long&)` instead of `long labs(long)`.

`std::vector` seems like a suitable type for a public API and return values. In contrast to `SmallVector`, one does not need to encode an arbitrary small-size and needs to be moved for the caller anyway. https://youtu.be/fHNmRkzxHWs?t=2025

However, if you have some other preference, I would like to hear it.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1205
+  DeleteDeadBlocks(BBVec);
+}
+
----------------
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`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1234
+  // body, these BasicBlocks will be sunk into the loop nest body. That is,
+  // these instructions may be executed more often than before the tiling.
+  // TODO: It would be sufficient to only sink them into body of the
----------------
ftynse wrote:
> What happens if they have side effects?
The interface (and OpenMP 5.1) specifies that loop nest being tiled are perfectly nested, i.e. we can assume there are no instructions that have side-effects.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1250
+  Builder.restoreIP(OutermostLoop->getPreheaderIP());
+  SmallVector<Value *, 4> FloorCount, FloorEpilogues, FloorRems;
+  for (int i = 0; i < NumLoops; ++i) {
----------------
jdoerfert wrote:
> So far, I think, you could just replace `FloorEpilogues` with `FloorCount`. Do we expect that they are different later?
They were different at some point, missed that during clean-up.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1282
+  std::vector<CanonicalLoopInfo *> Result;
+  Result.reserve(NumLoops * 2);
+
----------------
jdoerfert wrote:
> In case we want to change the signature: At this point, I think, you only use `Loops` to delete some code. Maybe we shouldn't, or even if we do we could still reuse `Loops` to return the new ones?.
I left a comment about this here: https://reviews.llvm.org/D92974#inline-867960

I don't really want to bake into the interface which CanonicalLoopInfo* objects are reused, that's an implementation detail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92974