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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan marked 8 inline comments as done.
kiranchandramohan added inline comments.


================
Comment at: mlir/examples/toy/Ch6/CMakeLists.txt:55
+  MLIROpenMP
+  LLVMFrontendOpenMP
   MLIRStandardOps
----------------
kiranchandramohan wrote:
> 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.
As mentioned before, this was required to the dependence of MLIRTranslation on LLVMFrontendOpenMP.


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


================
Comment at: mlir/tools/mlir-cpu-runner/CMakeLists.txt:9
+  MLIROpenMP
+  LLVMFrontendOpenMP
   MLIRTargetLLVMIR
----------------
kiranchandramohan wrote:
> 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.
I had to add LLVMFrontendOpenMP to FULL_LIBS used in whole_archive_link in mlir/tools/mlir-translate/CMakeLists.txt since libMLIRTranslation appears after LLVMFrontendOpenMP in the link line.


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

https://reviews.llvm.org/D72962