[PATCH] D141010: OpenMPOpt: Check nested parallelism in target region

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:602
+  /// Flag that indicates if the kernel has nested Parallelism
+  bool NestedParallelism = false;
+
----------------
randreshg wrote:
> tianshilei1992 wrote:
> > randreshg wrote:
> > > tianshilei1992 wrote:
> > > > Is it good enough to just have two states? Is it possible that we can't tell if a parallel region is nested or not, or do we just conservatively take it as `true`?
> > > What others states we might possibly have? The only way we can't tell if a parallel region is nested or not is when we have functions with parallel regions in another TU, for this case we conservatively take it as `false`, for all the other cases this approach should work.
> > If a function containing a parallel region can be reached from different paths, and the parallel region may or may not be nested, that would be the case right? What's more, I suppose we could do more aggressive optimization if we do not have nested parallelism, which means the conservative way would be to take it as a `true`. Did I misunderstand anything here?
> The case is summarized in the remark [[ https://openmp.llvm.org//remarks/OMP133.html#omp133 | omp133]]. This is the case when a function inside the target region may contain parallel regions but since we don't have information about it, we assume it has no nested parallelism
We start optimistic, so assuming no nested parallelism is right.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:689
     ReachedUnknownParallelRegions ^= KIS.ReachedUnknownParallelRegions;
+    NestedParallelism = KIS.NestedParallelism ? true : NestedParallelism;
     return *this;
----------------
Also known as
`NestedParallelism |= KIS.NestedParallelism`


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:3344
+        ConstantInt::get(Int1Ty, NestedParallelism ? 1 : 0),
+        Kernel->getName() + "_nested_parallelism");
+
----------------
Use an int8 type, int1 is always tricky. 


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:4370
+        if (mayContainParRegion)
+          NestedParallelism = true;
         break;
----------------
Don't use magic constants (5, above), that's why we have these variables (WrapperFunctionArgNo). Also, start at the wrapper, so `ParallelRegion`. The DepClassTy should be optional.
Style for `mayCon...`, probably just remove the variable:
`NestedParallelism |= !FnAA...`


================
Comment at: llvm/test/Transforms/OpenMP/nested_parallelism.ll:4
+target triple = "nvptx64"
+
+%struct.ident_t = type { i32, i32, i32, i32, ptr }
----------------
Put the C code as a comment in here so it's easier to read the IR.


================
Comment at: llvm/test/Transforms/OpenMP/nested_parallelism.ll:342
+  ret void
+}
+
----------------
A lot of this code is really not needed. Maybe start with a simpler example or simplify the IR.


================
Comment at: llvm/test/Transforms/OpenMP/nested_parallelism.ll:417
+attributes #10 = { convergent nounwind }
+attributes #11 = { nocallback nofree nosync nounwind willreturn memory(argmem: readwrite) }
+
----------------
Remove all attributes.


================
Comment at: llvm/test/Transforms/OpenMP/nested_parallelism.ll:440
+!15 = !{!16, !16, i64 0}
+!16 = !{!"any pointer", !13, i64 0}
+;.
----------------
Remove the TBAA metadata and everything we don't need. !0 - !3 and !6 and !7 are needed. Adjust the offload_info and nvvm annotations, and module.flags accordingly.


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D141010