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

[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 Left some more comments that can be addressed before the commit. Otherwise LGTM.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:693
+      Extractor.findAllocas(CEAC, SinkingCands, HoistingCands, CommonExit);
+      Extractor.findInputsOutputs(Inputs, Outputs, SinkingCands);
+
----------------
If you just want the users of instructions in `SeqStartBB` outside of that BB we don't need the extractor logic which probably does a lot more than we need. If you agree, maybe something like this:

```
for (Instruction &I : *SeqStartBB) {
  SmallPtrSet<Instruction *, 4> OutsideUsers;
  for (User &Usr : I.users()) {
    Instruction &UsrI = *cast<Instruction>(Usr);
    if (UsrI.getParent() != SeqStartBB)
      OutsideUsers.insert(&UsrI);
  }
  if (OutsideUsers.empty())
    continue;
  // Do the stuff as below.
}
```


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:917
+
+        if (isa<CallInst>(&I)) {
+          if (IsBeforeMergableRegion) {
----------------
Early exit:
```
 if (!isa<Call>)
   return true;
```


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:920
+            CallInst *CI = cast<CallInst>(&I);
+            Function *CalledFunction = CI->getCalledFunction();
+            // Return false (unmergable) if the call before the parallel
----------------
`if (!CalledFunction) return false;`


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:926
+            // TODO: ICV tracking to detect compatibility.
+            for (auto RFI : UnmergableCallsInfo) {
+              if (CalledFunction == RFI.Declaration)
----------------
`const auto &RFI : `


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:966
+          }
+        }
+
----------------
Nit: pre-increment everywhere above: `++It`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90909