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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

Thanks! I have a couple of comments.



================
Comment at: mlir/examples/toy/Ch6/CMakeLists.txt:55
+  MLIROpenMP
+  LLVMFrontendOpenMP
   MLIRStandardOps
----------------
Why are these changes necessary? The code doesn't seem to be modifying the tutorial. 


================
Comment at: mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:102
   std::unique_ptr<llvm::Module> llvmModule;
+  std::unique_ptr<llvm::OpenMPIRBuilder> OMPBuilder;
 
----------------
Please add a comment for this. "Original and translated module" does not describe it appropriately.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:342
 
+  if (opInst.getDialect()->getNamespace() == "omp") {
+    if (existsOrCreateOpenMPIRBuilder()) {
----------------
`opInst.getDialect() == context->getRegisteredDialect<OpenMPDialect>()` looks cleaner to me, even if it does the same comparison under the hood.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:344
+    if (existsOrCreateOpenMPIRBuilder()) {
+      if (dyn_cast<omp::BarrierOp>(opInst)) {
+        OMPBuilder->CreateBarrier(builder.saveIP(), llvm::omp::OMPD_barrier);
----------------
`isa<omp::BarrierOp>`


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:348
+      }
+      return opInst.emitError("unsupported or non-OpenMP operation: ")
+             << opInst.getName();
----------------
you cannot have non-OpenMP operation here, the outermost `if` checked that it belongs to the OpenMP dialect.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:350
+             << opInst.getName();
+    }
+  }
----------------
Nit: I'd provide more specific error message in case OpenMP is not enabled. Currenty, we will just fail to translate with the generic "unsupported or non-LLVM operation".


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:650
+bool ModuleTranslation::existsOrCreateOpenMPIRBuilder() {
+  if (!enableOpenMP)
+    return false;
----------------
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.


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


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

https://reviews.llvm.org/D72962