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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
