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

[Giorgis Georgakoudis via Phabricator via llvm-commits]

ggeorgakoudis added inline comments.


================
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(
----------------
ggeorgakoudis wrote:
> 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?
I had a quick look in the OpenMP runtime implementation to answer my question. The `proc_bind` setting is handled within the implementation of the fork call at team creation, so calling it within an executing parallel region should have no effect on it. Does it make the most sense to just emit a call to `__kmpc_push_proc_bind`, falling back to default binding? That will invalidate `proc_bind` settings on merged parallel regions, is that within OpenMP specification?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83635