[PATCH] D72962: [MLIR, OpenMP] Translation of OpenMP barrier construct to LLVM IR

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:107
+  /// Builder for LLVM IR generation of OpenMP constructs.
+  std::unique_ptr<llvm::OpenMPIRBuilder> OMPBuilder;
+
----------------
Variables names should be camelCase.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:379
 
+  if (opInst.getDialect() ==
+      opInst.getContext()->getRegisteredDialect<omp::OpenMPDialect>()) {
----------------
When adding more OMP operations(i.e. in a followup) can we split translating these operations into a different function? This one is already getting quite large.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:380
+  if (opInst.getDialect() ==
+      opInst.getContext()->getRegisteredDialect<omp::OpenMPDialect>()) {
+    if (!OMPBuilder) {
----------------
We should really pre-compute this. Looking up a registered dialect involves grabbing a lock, and iterating O(N) over all dialects.


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

https://reviews.llvm.org/D72962