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

[Johannes Doerfert via Phabricator via cfe-commits]

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


================
Comment at: clang/include/clang/Basic/OpenMPKinds.h:29
-  OMPD_unknown
+  OMPD_unknown,
 };
 
----------------
These changes do not impact Clang nor do they remove functionality (if OPENMP_DIRECTIVE is defined it will work as it did before) but they are necessary to make the enum values in Clang and the OpenMPIRBuilder equal.

The reason is that the OPENMP_DIRECTIVE and OPENMP_DIRECTIVE_EXT definitions in `OpenMPKinds.def` are interleaved but for the OpenMPIRBuilder I was hoping not to separate them logically. So the first three changes adjust Clang towards this simpler model that is still as powerful as the old one was but which changes the order of the enums.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:3490
+  }
+
   if (!CGF.HaveInsertPoint())
----------------
The first extension could be for Clang to communicate the cancellation blocks to the OMPBuilder which would allow us there to handle all barriers. Note: There is no need for a callback (in the long run) as cancellation points will be handled by the OMPBuilder eventually.


================
Comment at: clang/test/OpenMP/for_firstprivate_codegen.cpp:291
 // CHECK: define internal void [[TMAIN_MICROTASK]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i32* dereferenceable(4) %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [2 x [[S_INT_TY]]]* dereferenceable(8) %{{.+}}, [[S_INT_TY]]* dereferenceable(4) %{{.+}})
+// CHECK: [[GTID_CALL:%.+]] = call i32 @__kmpc_global_thread_num
 // Skip temp vars for loop
----------------
All test changes are because we do not check if there is an existing global thread ID value available through an argument. See the comment in `OpenMPIRBuilder::getThreadID` for further information and a proper way out.


================
Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:36
+#include "llvm/IR/OpenMPKinds.def"
+  };
+
----------------
The enum values are equivalent to the ones in Clangs `OpenMPDirectiveKind`, they have to be as we convert the latter to the former right now. At some point we won't need the latter but for now we should probably put a static assert here to verify the first and last enum values are equal.


================
Comment at: llvm/include/llvm/IR/OpenMPIRBuilder.h:58
+    IRBuilder<>::InsertPoint IP;
+  };
+
----------------
This struct needs to be extended with source location information in a way not tied to Clang. We wan to use that for two purposes: (1) generate `ident_t` with actual source locations embedded and (2) tell the IRBuilder to emit debug info if requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69785