[PATCH] D92974: [OpenMPIRBuilder] Implement tileLoops.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1177
+/// ones that are not reachable from the function.
+static void removeUnusedBlocksFromParent(ArrayRef<BasicBlock *> BBs) {
+  SmallPtrSet<BasicBlock *, 6> BBsToErase{BBs.begin(), BBs.end()};
----------------
This essentially replaces the CanonicalLoopInfo::eraseFromParent introduced in D90830. Erasing the BBs in the scope of only a single loop is unfortunately not enough to discover the BBs that have become orphaned after some of them are reused. It is possible, but error-prone for `tileLoops` to itself mark BBs that it reuses while constructing the new loop nest and delete the others. However, doing a fixipoint computation here is easier and I expect more methods consuming loop nests (such as `collapseLoops`) to face the same problem.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1235-1236
+  // these instructions may be executed more often than before the tiling.
+  // TODO: It would be sufficient to only sink them into body of the
+  // corresponding tile loop.
+  SmallVector<std::pair<BasicBlock *, BasicBlock *>, 4> InbetweenCode;
----------------
I discovered quite late that handling of such in-between code is necessary (the `TileNestedLoopsWithBounds` test inserts them to compute the loop counter value from the canonical induction variable), previously assuming that perfectly nested loops do not have such thing.

The approach of creating completely new floor and tile loops has the advantage that I can give it nice `floor<i>` and `tile<i>` names and can generate them essentially with the same outer-to-inner mechanism.

In contrast, reusing the original CanonicalLoopNest as new tile loops would given us the handling of in-between code for free, in the manner mentioned in the TODO (The in-between code is already in-between the tile loops).


================
Comment at: llvm/lib/IR/BasicBlock.cpp:328
   // Return early if there are no PHI nodes to update.
-  if (!isa<PHINode>(begin()))
+  if (empty() || !isa<PHINode>(begin()))
     return;
----------------
This change is necessary for `createCanonicalLoop` to be able to insert a loop at the end of a BasicBlock that does not yet have a terminator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92974