[PATCH] D83635: [OpenMPOpt][WIP] Merge parallel regions

[Johannes Doerfert via Phabricator via llvm-commits]

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

A few minor nits, otherwise LGTM.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:524
+      Changed |= mergeParallelRegions();
+      Changed |= deduplicateRuntimeCalls();
+    }
----------------
run this conditional on the result of merge:
```
if (merge...()) {
 deduplicate..
 Changed = true;
}
```


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:689
+      // TODO: Verify proc bind matches and use the value.
+      // TODO: Check the cancellable flag.
+      InsertPointTy AfterIP = OMPInfoCache.OMPBuilder.CreateParallel(
----------------
I think both todos are handled at this point.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:725
+             OutlinedFn->getAttributes().getParamAttributes(1))
+          NewCI->addParamAttr(1, A);
+        for (unsigned U = CallbackFirstArgOperand, E = CI->getNumArgOperands();
----------------
No need for this. Any pass that might use this will make the connection on its own and treating the two arguments special is weird.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:729
+          for (const Attribute &A : CI->getAttributes().getParamAttributes(U))
+            NewCI->addParamAttr(U - 1, A);
+
----------------
Can you use the variables instead of 1 here.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:820
+      RFI.clearUsesMap();
+      OMPInfoCache.collectUses(RFI, /* CollectStats */ false);
+    }
----------------
I think we have to collect the uses of the barrier as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83635