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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Sorry for my slow review. I added a bunch of comments, mostly minor things and requests for some extra tests.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:631
+                        << " parallel regions in " << OriginalFn->getName()
+                        << "\n");
+
----------------
We should also emit a remark here. And one pointing at each of the merged parallel regions.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:598
+
+    BasicBlock *StartBB, *EndBB;
+    auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
----------------
Nit: Initialize them with `nullptr` and place an assert at the use site, makes it easier to trace a bug if we ever mess up.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:638
+      StartBB =
+          SplitBlock(BB, MergableCIs.front(), DT, LI, nullptr, "expnd_omp_pr");
+
----------------
Nit: For some reason it is "common" to use `abc.dce.xyz` as naming scheme for blocks (I think). Maybe we should go with something similar to the OMPIRBuilder, i.a., `omp.par.expanded`. Just a suggestion.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:647
+      // TODO: Verify proc bind matches and use the value.
+      // TODO: Check the cancellable flag.
+      InsertPointTy AfterIP = OMPInfoCache.OMPBuilder.CreateParallel(
----------------
I think we need some tests for these at some point. One with proc-binds and one with combinations of cancellable and non-cancellable regions.  I think it "should just work" but we need to make sure the resulted expanded parallel region does what we would expect. I would assume cancellable is actually not a problem at all, proc-bind specified for the first of two that are merged might be. I expect we merge and both now have have the proc-bind now for the entire region. This is not ideal. TBH, the real problem is that `__kmpc_push_proc_bind` exists and the value is not passed to `__kmpc_fork_call`. On top of that, we only generate the `__kmpc_push_proc_bind` if we need it. That leaves the (potential) situation in which the call is present but we don't see it. So we need to either use the ICV tracking facility here or change the way we generate code. I guess always emitting `__kmpc_push_proc_bind` is the easiest way to handle this. Then we can look for the call and verify the binding is the same. WDYT?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:697
+              OMPD_parallel);
+          OMPInfoCache.OMPBuilder.finalize();
+        }
----------------
Do we need to finalize the builder every time here (or at all)?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:721
+
+    // Helper function that performs that identifes sequences of
+    // __kmpc_fork_call uses in a basic block.
----------------
typo


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:729
+                        << BB2PRMap.size() << " blocks in " << F.getName()
+                        << "\n");
+
----------------
Do we want to print this for each use or once at the end?


================
Comment at: llvm/test/Transforms/OpenMP/parallel_region_merging.ll:318
+!0 = !{!1}
+!1 = !{i64 2, i64 -1, i64 -1, i1 true}
----------------
The update test script cannot add check lines for new functions yet. Can you manually add some minimal check lines to ensure the new function we create looks as expected, e.g., has regular calls to the former parallel regions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83635