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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:650
+bool ModuleTranslation::existsOrCreateOpenMPIRBuilder() {
+  if (!enableOpenMP)
+    return false;
----------------
ftynse wrote:
> This should not be controlled by a command-line flag. It makes this flow unusable as a library (e.g., if the integration does not have a command-line interface). Pass the option to the constructor of ModuleTranslation (or whatever the way it is currently created) instead and default-initialize it from the command-line flag.
I don't think we need the switch to begin with. If "omp" ops are supposed to be lowered we need a OMPBuilder. There is no reason not to build one and run into an assertion instead. With the lazy generation of the OMPBuilder we can really just remove the command line flag and get sane behavior w/ and w/o OpenMP ops.


================
Comment at: mlir/tools/mlir-cpu-runner/CMakeLists.txt:9
+  MLIROpenMP
+  LLVMFrontendOpenMP
   MLIRTargetLLVMIR
----------------
ftynse wrote:
> Do we really need whole archive link for LLVMFrontendOpenMP, i.e. is there some static registration in that library?
>  i.e. is there some static registration in that library?

Should not be.


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

https://reviews.llvm.org/D72962