[Openmp-commits] [PATCH] D142569: [OpenMP] Introduce kernel environment

Johannes Doerfert via Phabricator via Openmp-commits openmp-commits at lists.llvm.org
Tue Feb 14 22:32:54 PST 2023

jdoerfert added inline comments.

Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3888
+  Function *Kernel = Builder.GetInsertBlock()->getParent();
   Function *Fn = getOrCreateRuntimeFunctionPtr(
That is not the kernel, at least not when clang emits a debug wrapper as part of -g, I think.
We have one workaround for this problem already but we need to provide a generic way. I'll add something.

Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3907
+  GlobalVariable *KernelEnvironment = new GlobalVariable(
+      M, KernelEnvironmentTy, /* IsConstant */ true,
+      GlobalValue::ExternalLinkage, KernelEnvironmentInitializer,
We can't have it constant as a whole. We need a constant part and a non-constant part. Let's add them like that from the very beginning. 
I'd suggest we have a third struct type that is references from the kernelenv. It contains for now only the Level value, but we can/will add more things. It's an independent global such that we can make the kernelenv constant.

Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:212
Maybe close the namespace here.

Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:3717
+    if (KernelEnvC) {
+      ConstantInt *MayUseNestedParallelismC =
Shouldn't we always have one here?

Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:3724
+      setMayUseNestedParallelismOfKernelEnvironment(
+          NewMayUseNestedParallelismC);
This should happen above in the initializer. We should assume no-nested-parallelism and go back on that assumption if need be.

Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:4452
+          AA.setExecModeOfKernelEnvironment(
+              KernelInfo::getExecModeFromKernelEnvironment(ExistingKernelEnvC));
+      }
And here you need to check nested-parallelism then too.

Comment at: openmp/libomptarget/DeviceRTL/src/Configuration.cpp:57
+  return !__omp_rtl_assume_no_nested_parallelism ||
+         state::getKernelEnvironment().Configuration.MayUseNestedParallelism;
tianshilei1992 wrote:
> jdoerfert wrote:
> > This looks wrong. We want either flag to allow us to remove nested parallelism handling. So 
> > `__omp_rtl_assume_no_nested_parallelism ` is good enough, and
> > `!state::getKernelEnvironment().Configuration.MayUseNestedParallelism` is good enough.
> Yeah, since I already updated `OpenMPOpt.cpp`, `__omp_rtl_assume_no_nested_parallelism` will not be generated.
`__omp_rtl_assume_no_nested_parallelism` is generated by clang, the value depends on `-fopenmp-assume-no-nested-parallelism`

  rG LLVM Github Monorepo



More information about the Openmp-commits mailing list