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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

(This likely need a rebase)



================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:650
+bool ModuleTranslation::existsOrCreateOpenMPIRBuilder() {
+  if (!enableOpenMP)
+    return false;
----------------
jdoerfert wrote:
> kiranchandramohan wrote:
> > 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 ?
> FWIW, running OpenMP programs "sequentially", in the sense that you *ignore* OpenMP, is only sound in *special cases*. Various OpenMP directives change the program semantics (at least potentially) such that ignoring OpenMP directives is not a strategy we should recommend (and we most certainly cannot "support" it in a sane way). You can run them "sequentially" by setting the appropriate upper bounds to 1 though.
For now this is fine with me to skip the flag entirely.


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

https://reviews.llvm.org/D72962