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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:29
+
+extern llvm::cl::opt<bool> enableOpenMP;
 
----------------
cl::opt are only intended for testing, I wouldn't expect it in library code.


================
Comment at: mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp:25
+    "enable-openmp", llvm::cl::desc("Enable translation of OpenMP dialect Ops"),
+    llvm::cl::init(false)};
+
----------------
jdoerfert wrote:
> 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?
Lazily creating the `OMPBuilder` on demand seems like ideal to me? It there a downside?

What about always creating it eagerly: is it that heavy/costly?


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

https://reviews.llvm.org/D72962