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

[Johannes Doerfert via Phabricator via Openmp-commits]

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
+KERNEL_ENVIRONMENT_CONFIGURATION_IDX(ExecMode, 2)
+
+#undef KERNEL_ENVIRONMENT_CONFIGURATION_IDX
----------------
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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142569