[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 6 inline comments as done.
jdoerfert added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.def:215
 LANGOPT(OpenMPCUDAMode    , 1, 0, "Generate code for OpenMP pragmas in SIMT/SPMD mode")
+LANGOPT(OpenMPNewCodegen  , 1, 0, "Use the experimental OpenMP-IR-Builder codegen path.")
 LANGOPT(OpenMPCUDAForceFullRuntime , 1, 0, "Force to use full runtime in all constructs when offloading to CUDA devices")
----------------
Meinersbur wrote:
> Shouldn't this rather be a CODEGENOPT (`CodeGenOptions.def`)?
This can/should be changed for all relevant fopenmp options above in a dedicated patch. My option mimics other that exist.


================
Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186
+///{
+
+#ifndef OMP_IDENT_FLAG
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > JonChesterfield wrote:
> > > Sharing constants between the compiler and the runtime is an interesting subproblem. I think the cleanest solution is the one used by libc, where the compiler generates header files containing the constants which the runtime library includes.
> > I'd prefer not to tackle this right now but get this done first and revisit the issue later. OK?
> I don't think this is a good solution. It means that libomp cannot built built anymore without also building clang. Moreover, the values cannot be changed anyway since it would break the ABI.
> 
> I'd go the other route: The libomp defines what it's ABI is, the compiler generates code for it. 
This patch doesn't change what we do, just where. The numbers are hard coded in clang now. Let's start a discussion on the list and if we come up with a different scheme we do it after this landed.


================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:120
+  if (!SrcLocStr) {
+    Constant *Initializer =
+        ConstantDataArray::getString(M.getContext(), LocStr);
----------------
ABataev wrote:
> `auto *`?
No benefit and less clear, I'll stick with the type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785