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

[Johannes Doerfert via Phabricator via llvm-commits]

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


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:23-24
 enum OpenMPDirectiveKind {
-#define OPENMP_DIRECTIVE(Name) \
-  OMPD_##Name,
 #define OPENMP_DIRECTIVE_EXT(Name, Str) \
----------------
ABataev wrote:
> Better to make in a separate NFC patch.
Agreed, will do.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3483
+  auto *OMPRegionInfo = dyn_cast_or_null<CGOpenMPRegionInfo>(CGF.CapturedStmtInfo);
+  if (!EmitChecks || ForceSimpleCall || !OMPRegionInfo ||
+      !OMPRegionInfo->hasCancel()) {
----------------
ABataev wrote:
> I would add an option, something like `-fopenmp-experimental` for all changes with OpenMPBulder unless it at least matches in functionality/performance with the existing solution.
We need the experimental flag eventually so I can add it here.

FWIW, there is only one functional difference for barriers and I will actually use this as a first task of the OpenMP-aware middle-end optimization. A new patch will be put for review shortly and once accepted we get better `om_get_thread_num` call merging than we could have in the front-end anyway.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3493
     return;
+
   // Build call __kmpc_cancel_barrier(loc, thread_id);
----------------
ABataev wrote:
> Better to remove all these formatting changes from the patch.
Sure.


================
Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:33
+  /// IDs for all OpenMP directives.
+  enum DirektiveKind {
+#define OMP_DIRECTIVE(Enum, ...) Enum,
----------------
ABataev wrote:
> 1. `DirectiveKind`
> 2. Is it possible to merge these 2 types in LLVM and in clang into 1?
1. Agreed.
2. Yes! Clang needs to eventually use the one here. I can already make that change now or we wait a bit longer and have both coexist. The change would probably be something along the lines of: `using OpenMPDirectiveKind = llvm::OpenMPIRBuilder::DirectiveKind` in clang instead of the enum definition. We probably also want to include `llvm/IR/OpenMPKinds.def` where `OPENMP_DIRECTIVE_EXT` is now used. 


================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:108
+    DefaultIdent->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+    DefaultIdent->setAlignment(Align(8));
+  }
----------------
ABataev wrote:
> Is this correct for 32-bit platforms? Maybe rely on the frontend when creating structures and all other required feature things?
None of these properties, e.g., the alignment, should ever be wrong. It might differ from what we have now on a certain platform but it should not be wrong or incompatible. That being said, we can certainly think about asking the frontend to generate global declarations for us (types should be the same). I don't see an immediate benefit of doing so but if we have a reason it is certainly doable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785