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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

In D72962#1872745 <https://reviews.llvm.org/D72962#1872745>, @compiler_developer wrote:

> In D72962#1872679 <https://reviews.llvm.org/D72962#1872679>, @kiranchandramohan wrote:
>
> > In D72962#1872562 <https://reviews.llvm.org/D72962#1872562>, @compiler_developer wrote:
> >
> > >
> >
> >
> > Thanks for your comments.
> >
> > > **MLIROpenMP** and **LLVMFrontendOpenMP** libraries looks like ** an additional dependency for MLIR users who wants LLVM lowering and doesn't want anything to do with the OpenMP**! I think Toy example cmake file changes is one of the early symptoms.
> >
> > One way to handle that would be to go back to the first version of this patch which implements translation by deriving from  public LLVM::ModuleTranslation.
>
>
> I think conversion patterns would be more appropriate. But, I think, that is dependent on the decision to emit LLVM IR instead of LLVM Dialect.


OK. I was pointing out that if we do not want these dependencies then we have a way to achieve that by deriving from the LLVM Module translator.

> 
> 
>>> Can't OpenMP dialect have a separate LLVM dialect lowering similar to all other dialects in MLIR? I think it makes sense to lower OpenMP constructs in LLVM dialect rather than having a OpenMPIRBuilder based on LLVM IR.
>> 
>> We decided to use the OpenMPIRBuilder since that way we will be able to reuse existing code in the llvm-project (clang) for OpenMP code generation.
> 
> Given that MLIR has potential to support various parellel IR semantics as different dialects, I think having an LLVM dialect lowering for OpenMP would be much cleaner solution. Also, shouldn't there be minimum difference between the LLVM Dialect and LLVM IR. If I am not wrong the goal (or atleast it definitely makes sense) is to have a tablegen based LLVM Dialect to LLVM IR conversion and viceversa. Having OpenMP dialect would affect it?

LLVM dialect and its conversion will not be affected by the OpenMP dialect or the changes in the translation library to lower the dialect. The OpenMP translation will only kick in if there are OpenMP dialect operations during translation.

We did consider lowering to the LLVM dialect as can bee seen in Slide 8 of the presentation (link below) and my first mail to mlir list (link below, see also replies from mlir developers). But given that there is the OpenMPIRBuilder (which provides a good interface to generate LLVM IR for OpenMP constructs), we felt it would be best to use the OpenMPIRBuilder rather than to re-implement that work in MLIR. 
https://drive.google.com/file/d/1vU6LsblsUYGA35B_3y9PmBvtKOTXj1Fu/view?usp=sharing
https://groups.google.com/a/tensorflow.org/d/msg/mlir/4Aj_eawdHiw/BSpWvR6zCAAJ

If there was no OpenMP codegen present in LLVM then I would have also have preferred to lower to LLVM dialect. Maybe in future when Clang also uses MLIR, we can modify OpenMPIRBuilder to generate LLVM dialect.

> 
> 
>>> What happens in case of LLM IR + OpenMP to LLVM dialect conversion? OpenMP dialect will be created too? Or else the LLVM IR to/from LLVM dialect (+ OpenMP dialect) will behave differently!
>> 
>> I believe we can support this option under a flag. With the flag OpenMP + LLVM dialect is created. Without the flag only LLVM dialect will be created.
> 
> But, would it be possible to extract the OpenMP operations from the LLVM IR? I think that is a difficult problem to solve, especially when the IR is optimized. Also, I dont think it can be under a flag as conversion should be round-trippable?

Yes, this will be a more difficult problem to solve.


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

https://reviews.llvm.org/D72962