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

[Giorgis Georgakoudis via Phabricator via llvm-commits]

ggeorgakoudis marked 12 inline comments as done.
ggeorgakoudis added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:514
     Changed |= deleteParallelRegions();
+    Changed |= deduplicateRuntimeCalls();
+    Changed |= mergeParallelRegions();
----------------
I have intentionally added re-running deduplication before merging. It helps removing emitted calls to `__kmpc_global_thread_num`, and that enables merging of parallel regions by removing redundant in-between instructions (at least at this point that merging is very defensive)


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:598
+
+    BasicBlock *StartBB, *EndBB;
+    auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
----------------
jdoerfert wrote:
> 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.
Asserts on uses within BodyGenCB 


================
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(
----------------
jdoerfert wrote:
> 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?
This is tricky. What if merged parallel regions have different `proc_bind` clauses? Is it possible to call `__kmpc_push_proc_bind` within a parallel region, during parallel execution?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:697
+              OMPD_parallel);
+          OMPInfoCache.OMPBuilder.finalize();
+        }
----------------
jdoerfert wrote:
> Do we need to finalize the builder every time here (or at all)?
This finalize call is indeed unnecessary. The only call to finalize needed is when outlining the new, merged parallel region.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:729
+                        << BB2PRMap.size() << " blocks in " << F.getName()
+                        << "\n");
+
----------------
jdoerfert wrote:
> Do we want to print this for each use or once at the end?
I have moved debug prints in the loop of the BB2PRMap data structure and made it more informative (add how many parallel mergeable parallel regions have been found)


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:631
+                        << " parallel regions in " << OriginalFn->getName()
+                        << "\n");
+
----------------
jdoerfert wrote:
> We should also emit a remark here. And one pointing at each of the merged parallel regions.
We emit a remark in line 708 that identifies merged parallel regions. What do you have in mind? Is it a single remark that contains identifiers for all merged regions?


================
Comment at: llvm/test/Transforms/OpenMP/parallel_region_merging.ll:318
+!0 = !{!1}
+!1 = !{i64 2, i64 -1, i64 -1, i1 true}
----------------
jdoerfert wrote:
> sstefan1 wrote:
> > jdoerfert wrote:
> > > 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?
> > Maybe I can take a look at that since I'm now somewhat familiar with the script.
> there is a patch for the _cc_ script already: D83004. I suggested there to make it generic (common.py) for _cc_ and the opt script, maybe we can do it in 2 steps instead. < @greened 
I see the patch is accepted but not yet committed. Waiting for it to be upstreamed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83635