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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev 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) \
----------------
Better to make in a separate NFC patch.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3483
+  auto *OMPRegionInfo = dyn_cast_or_null<CGOpenMPRegionInfo>(CGF.CapturedStmtInfo);
+  if (!EmitChecks || ForceSimpleCall || !OMPRegionInfo ||
+      !OMPRegionInfo->hasCancel()) {
----------------
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.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3486
+    CGF.CGM.getOpenMPIRBuilder().emitOMPBarrier(
+        {CGF.Builder.saveIP()}, llvm::OpenMPIRBuilder::DirektiveKind(Kind),
+        EmitChecks);
----------------
`DirectiveKind`


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


================
Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:33
+  /// IDs for all OpenMP directives.
+  enum DirektiveKind {
+#define OMP_DIRECTIVE(Enum, ...) Enum,
----------------
1. `DirectiveKind`
2. Is it possible to merge these 2 types in LLVM and in clang into 1?


================
Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:108
+    DefaultIdent->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+    DefaultIdent->setAlignment(Align(8));
+  }
----------------
Is this correct for 32-bit platforms? Maybe rely on the frontend when creating structures and all other required feature things?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785