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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

No need to wait for me on this review but I added two more comments.



================
Comment at: mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp:25
+    "enable-openmp", llvm::cl::desc("Enable translation of OpenMP dialect Ops"),
+    llvm::cl::init(false)};
+
----------------
You could just check if `OMPBuilder` is initialized in order to keep this cmd line option private to a single file. Arguably, you could also create a method `getOMPBuilder()` which create one if none is present as soon as an OpenMP op is discovered. Maybe I'm missing some intended interaction of OpenMP ops and this flag set to false?


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:340
+      llvm::OpenMPIRBuilder::LocationDescription Loc({builder.saveIP()});
+      OMPBuilder->CreateBarrier(Loc, llvm::omp::OMPD_barrier);
+      return success();
----------------
Nit: If you want it less verbose, `OMPBuilder->CreateBarrier(builder.saveIP(), llvm::omp::OMPD_barrier);` should work too.


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

https://reviews.llvm.org/D72962