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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan marked 9 inline comments as done.
kiranchandramohan added a comment.

Thanks @ftynse for the comments. I will make a patch soon to address comments. The only question is regarding the necessity of the flag. Please see comment inline.



================
Comment at: mlir/examples/toy/Ch6/CMakeLists.txt:55
+  MLIROpenMP
+  LLVMFrontendOpenMP
   MLIRStandardOps
----------------
ftynse wrote:
> Why are these changes necessary? The code doesn't seem to be modifying the tutorial. 
Are you asking specifically for the whole-archive link or for target_link_libraries also?

I was getting some link errors since the OpenMP symbols are needed now.


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


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


================
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);
----------------
ftynse wrote:
> `isa<omp::BarrierOp>`
Will do.


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


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:350
+             << opInst.getName();
+    }
+  }
----------------
ftynse wrote:
> 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".
OK.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:650
+bool ModuleTranslation::existsOrCreateOpenMPIRBuilder() {
+  if (!enableOpenMP)
+    return false;
----------------
jdoerfert wrote:
> 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.
I think one of the suggestions early on was that if there is no OpenMP support (indicated via the switch/flag) then someone can try to generate code with other parallel libraries (like pthreads) or maybe run sequentially (is that possible? would it be equal to ignoring the openmp op).

For now, is it OK to not have this flag as is being suggested by @jdoerfert ?


================
Comment at: mlir/tools/mlir-cpu-runner/CMakeLists.txt:9
+  MLIROpenMP
+  LLVMFrontendOpenMP
   MLIRTargetLLVMIR
----------------
jdoerfert wrote:
> 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.
I will check and remove if not needed.


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

https://reviews.llvm.org/D72962