[PATCH] D147219: [OpenMP][Flang][MLIR] Lowering of requires directive from MLIR to LLVM IR

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Thanks @skatrak for the detailed reply. It will be good to discuss this a bit more and once we are all convinced with the approach then we can go fast.

My reply is inline.



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1598-1617
+/// Converts the module-level set of OpenMP requires clauses into LLVM IR using
+/// OpenMPIRBuilder.
+static LogicalResult
+convertRequiresAttr(Operation &op, omp::ClauseRequiresAttr requiresAttr,
+                    LLVM::ModuleTranslation &moduleTranslation) {
+  auto *ompBuilder = moduleTranslation.getOpenMPBuilder();
+
----------------
skatrak wrote:
> kiranchandramohan wrote:
> > skatrak wrote:
> > > kiranchandramohan wrote:
> > > > Can this be an operation in the OpenMP dialect?
> > > Do you mean removing the `omp.requires` attribute and instead having an `omp.requires` operation to be specified similarly to this?
> > > ```
> > > module {
> > >   omp.requires { clauses = #omp<clause_requires reverse_offload|unified_shared_memory> }
> > >   ...
> > > }
> > > 
> > > ```
> > > Doing this would allow us to lower the operation in `OpenMPDialectLLVMIRTranslationInterface::convertOperation` instead of `OpenMPDialectLLVMIRTranslationInterface::amendOperation`, but the clauses from this directive must be used to modify the lowering of some other operations, not just to produce the registration function.
> > > 
> > > The current approach allows accessing this information through the `OffloadModuleInterface` to be passed along to the `OpenMPIRBuilder` (in ModuleTranslation.cpp, currently), but if we had an operation instead of an attribute we would have to search for it in the module to extract this, which I'm not sure it would be a cleaner solution. Also there would be no guarantee that no more than a single occurrence of this operation would be present within the module at all times (e.g. if the input is handwritten MLIR).
> > > 
> > > Or is your suggestion to lower every instance of a 'requires' directive in Fortran to its own `omp.requires` operation at the spot where it appears? Something like this:
> > > ```
> > > module {
> > >   func.func @f() {
> > >     omp.requires { clauses = #omp<clause_requires reverse_offload> }
> > >     ...
> > >   }
> > >   ...
> > >   func.func @g() {
> > >     omp.requires { clauses = #omp<clause_requires unified_shared_memory> }
> > >     ...
> > >   }
> > > }
> > > ```
> > > 
> > > The issue in that case becomes where to gather all of the clauses and lower to a single registration function in LLVM IR, since `convertOperation` would be called once per instance otherwise. The gathering of the clauses coming from different instances of 'requires' would currently be done in the bridge, so that the single module attribute is produced: D147218.
> > > 
> > > I suppose a solution to issues on both approaches would be to update the 'requires' flags in the `OpenMPIRBuilder::Config` structure in `convertOperation` and replace the `OpenMPIRBuilder::createRegisterRequires` function for a new `OpenMPIRBuilder::createOrUpdateRegisterRequires`, and make sure that only a single registration function is generated, eventually with the whole set of flags passed to the runtime (each subsequent call would update the 'flags' argument instead of creating the function and registering it as a constructor). I guess it wouldn't be an issue the fact that the config structure wouldn't have the complete set of clauses at all times, since they are supposed (and checked in semantics: D149337) to come before any operations that may be affected by them. But I'm also not sure about how clean this solution would be either.
> > > 
> > > Let me know what your thoughts are on whether any of these approaches to supporting 'requires' via an operation is preferable to an attribute, or if you have another idea, since any of these changes would result in multiple updates across all related pending patches so I want to make sure we agree on an approach before committing the time.
> > Thanks for the detailed reply and exploring the options.
> > 
> > My question was basically coming from the point of view that `convertRequiresAttr` is generating some llvm IR instructions. So, we could model these instructions as an operation in the OpenMP dialect. But you make a good point (point copied below).
> > 
> > > but if we had an operation instead of an attribute we would have to search for it in the module to extract this, which I'm not sure it would be a cleaner solution. Also there would be no guarantee that no more than a single occurrence of this operation would be present within the module at all times (e.g. if the input is handwritten MLIR).
> > 
> > Can we move this to `getOpenMPBuilder`? This way if we decide to add an omp.module then we can just replace the use of the interface with this module operation.
> > 
> > There are a few concerns regarding the amendOperation flow,
> > -> Just like you said about having the operation multiple times, the attribute can also appear multiple times. This can be handled by checking that this attribute is only present in the module.
> > ->  There is no equivalent to amendOperation in the the regular Dialect Conversion flow. If some one is using their own OpenMP libraries downstream and would like to use the Dialect Conversion mechanism, then there will be a problem.
> > -> The discardable nature of these attributes.
> > 
> > What do you think about this?
> > Can we move this to `getOpenMPBuilder`? This way if we decide to add an omp.module then we can just replace the use of the interface with this module operation.
> This could be done there, yes. However, it doesn't seem quite right to me lowering OpenMP-specific attributes inside of ModuleTranslation, even though this case would be a relatively small addition. In the case of adding an `omp.module` later I would assume that the proper spot to lower these attributes would be `OpenMPDialectLLVMIRTranslationInterface::convertOperation`, so it would have to be moved anyways. It makes sense to set up the `OpenMPIRBuilder::Config` in `getOpenMPBuilder` because it's where the OpenMP IR builder is created, so it's configured at the same time. But I'm not sure about doing any lowering there.
> 
> > There are a few concerns regarding the amendOperation flow,
> > -> Just like you said about having the operation multiple times, the attribute can also appear multiple times. This can be handled by checking that this attribute is only present in the module.
> That's a good point, if we keep this approach I'll add that check.
> 
> > -> There is no equivalent to amendOperation in the the regular Dialect Conversion flow. If some one is using their own OpenMP libraries downstream and would like to use the Dialect Conversion mechanism, then there will be a problem.
> I'm not quite familiar with the details of the regular dialect conversion flow, so I'd be happy if you could point out if I'm missing something here.
> 
> The OpenMP dialect currently only has implemented lowering to the LLVM IR dialect via the Dialect Conversion mechanism (mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp), which handles some but not all of the operations. This is currently triggered during flang compilation (flang/lib/Optimizer/CodeGen/CodeGen.cpp), resulting in an MLIR module with mostly LLVM IR dialect operations and possibly some remaining OpenMP dialect operations. This final MLIR representation is then translated to LLVM IR using the `ModuleTranslation` class, which in turn relies on all these dialect-specific translation interfaces that can define `amendOperation`.
> 
> If I understand it correctly, the Dialect Conversion mechanism on its own would not be enough to take advantage of all current support for OpenMP lowering regardless of whether we relied on dialect attributes or not, since some operations are translated directly to LLVM IR at the end of the MLIR flow. That last part is what's built on top of the `OpenMPIRBuilder`. So it feels to me like the more general issue really comes down to having the `OpenMPIRBuilder`, which translates OpenMP operations into LLVM IR instead of into LLVM IR dialect operations, rather than to using dialect attributes.
> 
> However, I do see how it would be possible to move logic from the `OpenMPIRBuilder` to the `ConvertOpenMPToLLVMPass`, but I'm not sure if we could do the same for OpenMP dialect attributes attached to operations from other dialects. The point I'm trying to make, I guess, is that we want to keep the `OpenMPIRBuilder`, so keeping OpenMP dialect-related information as attributes wouldn't make much of a difference as to the limitations of the Dialect Conversion mechanism. But again, I'm not that familiar with this, so if I'm wrong I'd be happy to be corrected and understand it better.
> 
> > -> The discardable nature of these attributes.
> According to the [MLIR Language Reference](https://mlir.llvm.org/docs/LangRef/#dialect-attribute-values), discardable attributes are just attributes that have semantics defined externally to the operation. I haven't been able to find any documentation related to the "discardability" of these attributes, but to me it should mean that the operation doesn't become invalid if it's taken away. So it could be the case that printing in text format some operation could ignore them, hence losing that information if that text is used to continue the compilation process. However, if we make sure that this is not the case for the `builtin.module` operation, there is no reason for any transformation/optimization/lowering passes outside of the OpenMP dialect to remove it, so I think it should not be a reason to decide against it.
> 
> I can see how these kinds of attributes could be lost if, during a pass, the operation they are attached to is replaced by something else without forwarding them or taking them into consideration to impact in some way the results of the process. And it wouldn't make sense to try to make every pass working on those operations aware of these attributes, so it makes sense that they would be discardable in this sense. However, in the case of the `builtin.module` I feel like this should not be a concern, since that kind of situation doesn't seem likely or even something that should be supported.
>> Can we move this to getOpenMPBuilder? This way if we decide to add an omp.module then we can just replace the use of the interface with this module operation.

> This could be done there, yes. However, it doesn't seem quite right to me lowering OpenMP-specific attributes inside of ModuleTranslation, even though this case would be a relatively small addition. In the case of adding an omp.module later I would assume that the proper spot to lower these attributes would be OpenMPDialectLLVMIRTranslationInterface::convertOperation, so it would have to be moved anyways. It makes sense to set up the OpenMPIRBuilder::Config in getOpenMPBuilder because it's where the OpenMP IR builder is created, so it's configured at the same time. But I'm not sure about doing any lowering there.

The concern here was that even though we are representing this as an attribute it seems to have an IR representation in LLVM IR. Also FWIU, it seems that we are generating a global constructor and not a metadata or a function attribute. The fact that it is a global constructor makes me feel that this should be provided as an option to the initializer/constructor in the OpenMPIRBuilder which can internally generate these.

>> There is no equivalent to amendOperation in the regular Dialect Conversion flow. If someone is using their own OpenMP libraries downstream and would like to use the Dialect Conversion mechanism, then there will be a problem.

> I'm not quite familiar with the details of the regular dialect conversion flow, so I'd be happy if you could point out if I'm missing something here.
>
> The OpenMP dialect currently only has implemented lowering to the LLVM IR dialect via the Dialect Conversion mechanism (mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp), which handles some but not all of the operations. This is currently triggered during flang compilation (flang/lib/Optimizer/CodeGen/CodeGen.cpp), resulting in an MLIR module with mostly LLVM IR dialect operations and possibly some remaining OpenMP dialect operations. This final MLIR representation is then translated to LLVM IR using the ModuleTranslation class, which in turn relies on all these dialect-specific translation interfaces that can define amendOperation.
>
> If I understand it correctly, the Dialect Conversion mechanism on its own would not be enough to take advantage of all current support for OpenMP lowering regardless of whether we relied on dialect attributes or not, since some operations are translated directly to LLVM IR at the end of the MLIR flow. That last part is what's built on top of the OpenMPIRBuilder. So it feels to me like the more general issue really comes down to having the OpenMPIRBuilder, which translates OpenMP operations into LLVM IR instead of into LLVM IR dialect operations, rather than to using dialect attributes.
> 
> However, I do see how it would be possible to move logic from the OpenMPIRBuilder to the ConvertOpenMPToLLVMPass, but I'm not sure if we could do the same for OpenMP dialect attributes attached to operations from other dialects. The point I'm trying to make, I guess, is that we want to keep the OpenMPIRBuilder, so keeping OpenMP dialect-related information as attributes wouldn't make much of a difference as to the limitations of the Dialect Conversion mechanism. But again, I'm not that familiar with this, so if I'm wrong I'd be happy to be corrected and understand it better.

This is a general point that I am making here. The OpenMP Dialect sits in MLIR tree and it will have users both in upstream llvm-project as well as downstream. The current method we have chosen to lower the OpenMP dialect is through the OpenMPIRBuilder at the translation phase. This is done so as to share the code with Clang and have a single implementation of OpenMP in llvm-project as far as possible. I am not suggesting a change in this flow.

But what we have to keep in mind is that there will be others who might be lowering the OpenMP dialect differently for e.g. because they might have their own OpenMP runtime. So, in the best case,  the design of the dialect should not be dependent on the OpenMPIRBuilder lowering, it should be general enough so that other users can also use it.

Now the problem here is that AFAIS, the Dialect conversions are usually defined for Operations and we do not have a counterpart for `amendOperation` that takes an Operation and an attribute. So how will a general user lower the OpenMP dialect? The best case here is again that there is an `omp.module` operation that when it undergoes DialectConversion will do all the required changes. With the current representation that we have chosen, they will have to add a custom Conversion Patter for the Builtin Module where the OpenMP attributes are interpreted. I don't know whether this is possible currently. Would be good to give this a try so that we know there is a way for DialectConversion users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147219