[PATCH] D90909: [OpenMPOpt][WIP] Expand parallel region merging

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Apologies for the delayed review.

Update the commit title and message.

I think we are basically set here. I left some minor comments, all but the Input alloca question and the deleted file are not worth a new round of review, I just want confirmation on those two points.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:188
+        }
+      }
       OI.EntryBB->moveBefore(&ArtificialEntry);
----------------
If this flag is not set there should not be any instructions, right? If so, we should add an assertion for that.

Style:
I would not use the iterators as you never actually reach the end() anyway. `while (true) { I = BB.front();` or `while(BB.size() > 1) {...` or something.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:638
 
+    auto CreateSequentialRegion =
+        [&](Function *OuterFn, BasicBlock *OuterPredBB,
----------------
Add a comment describing what this is doing on a high-level.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:670
+          // Find inputs/outputs to/from the sequential region. Convert inputs
+          // to pointer uses in the sequential region. Broadcast outputs to
+          // users outside the soon-to-be-merged region. Sinking/hoisting
----------------
Conversion to pointer happens in the IRBuilder (after D92189 landed). This means we can omit the input alloca stuff below, right?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:835
+      for (auto *It = MergableCIs.begin(); It != MergableCIs.end(); ++It) {
+        CallInst *ForkCI = *It;
+        for (Value *V : ForkCI->args())
----------------
Range loop :)


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:841
+      // in-between mergable parallel regions.
+      for (auto *It = MergableCIs.begin(); It != MergableCIs.end() - 1; ++It) {
+        Instruction *ForkCI = *It;
----------------
We usually recommend to compute the end iterator only once.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:954
 
+      auto isMergable = [&](Instruction &I, bool IsBeforeMergableRegion) {
+        if (isa<CallInst>(&I)) {
----------------
Add some comment explaining what this is doing on a high-level.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:974
+            if (!I.isLifetimeStartOrEnd())
+              return false;
+          }
----------------
A lot of heuristics could be used until we have an AAOpenMPRuntimeCalls attribute that determines if a call can result in an OpenMP runtime call.
Lifetime intrinisics is fine, any intrinsic should be OK actually.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:984
+        return true;
+      };
       // Find maximal number of parallel region CIs that are safe to merge.
----------------
Reverse the order ("early exits") to reduce indention:
````
if terminator
  ...
if !call
  ...
call handling.
```


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:986
       // Find maximal number of parallel region CIs that are safe to merge.
-      for (Instruction &I : *BB) {
+      for (auto It = BB->begin(); It != BB->end();) {
+        Instruction &I = *It;
----------------
If BB->end() stays the same capture it; `It = .., End = ...; It != End`


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1051
+      EndMasterRFI.clearUsesMap();
+      OMPInfoCache.collectUses(EndMasterRFI, /* CollectStats */ false);
     }
----------------
Can we make a helper for this, pass he OMPRTL kind and it does the 3 steps.


================
Comment at: llvm/test/Transforms/OpenMP/parallel_region_merging_legacy_pm.ll:412
-; CHECK-NEXT:    ret void
-;
----------------
Why was this file deleted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90909