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

[Eric Schweitz via Phabricator via llvm-commits]

schweitz added inline comments.


================
Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:650
+bool ModuleTranslation::existsOrCreateOpenMPIRBuilder() {
+  if (!enableOpenMP)
+    return false;
----------------
mehdi_amini wrote:
> 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.
There are several design choices one could try here.

Enable/disable OpenMP by:
  - some switch in the front-end (throws away the directives, very early)
  - creating/not creating the OpenMP dialect ops
  - keeping/erasing the OpenMP dialect ops (before the LLVMIR dialect)
  - translation modes of OpenMP dialect to LLVMIR dialect
  - maybe even closer to OpenMPIRBuilder (late, some sort of target support pushback?)

There is most likely trade offs here in terms of "user experience" and reporting diagnostics as to where and how to balance these approaches. Although supporting the "disabled vs. enabled OpenMP" use cases does seem like one worth providing with some mechanism.



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

https://reviews.llvm.org/D72962