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

[Sergio Afonso via Phabricator via llvm-commits]

skatrak added a comment.

Thank you Kiran for your suggestions. I'd be happy to follow any approach that's considered best for this, so I tried to explain the best I could my thinking about the points you raised and hopefully we can decide on a solution that can inform some of the other related work that's dealing with the same issues, as well.



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1080-1082
+  auto defaultAO = getAtomicDefaultMemOrder(opInst);
+  llvm::AtomicOrdering AO =
+      convertAtomicOrdering(readOp.getMemoryOrderVal(), defaultAO);
----------------
kiranchandramohan wrote:
> kiranchandramohan wrote:
> > skatrak wrote:
> > > kiranchandramohan wrote:
> > > > Should this be part of Semantics?
> > > It would be possible to gather the `atomic_default_mem_order` clause present in any 'requires' directives in the compilation unit, and then apply it (or 'monotonic', if not present) to all atomic operations with no memory order clauses as a semantic pass. I suppose that would also come together with making the memory order attribute mandatory in MLIR for these operations, since we would be handling the defaults prior to that. Is this a preferable approach?
> > Yes, that would be my preference. And it is fine for these instances of atomic operations to have the memory order attribute. Since these are anyway accepted as optional, there is no change in the operation definition.
> > 
> > I think in general, we should try to keep the translation as dumb and minimal as possible. So we can exclude this from here.
> The other option is to let the Openmpirbuilder handle this through the config. Why is this (the registration fn and the default ordering) done separately now?
> Yes, that would be my preference. And it is fine for these instances of atomic operations to have the memory order attribute. Since these are anyway accepted as optional, there is no change in the operation definition.
>
> I think in general, we should try to keep the translation as dumb and minimal as possible. So we can exclude this from here.

Sure, it makes sense, I will start working on that. If there are no instances of `requires atomic_default_mem_order`, should the semantic pass leave atomic operations unchanged or should it explicitly add the memory order clause with the default value?

> The other option is to let the Openmpirbuilder handle this through the config. Why is this (the registration fn and the default ordering) done separately now?

It made sense to split the 'requires' clauses into two attributes because there are different requirements for generating the registration function and for honoring the `atomic_default_mem_order`. The first should be produced if there are device constructs in the compilation unit, even if there are no 'requires' directives, whereas the second is honored as long as it appears in a 'requires' directive. By splitting these into two MLIR attributes it was possible to just add the ones that applied and avoid generating the registration function when it shouldn't be.

With regards to the way in which this patch currently picks up the value of the `omp.atomic_default_mem_order`, I agree that this could have been moved to the `OpenMPIRBuilder` and updated the `createAtomic...` functions to take that into account if an explicit memory order was not given. I only added it where I did because that's where the default was being picked up already, so it required less code changes. I can make this change instead of moving the logic to semantics, but I understand that the other approach is preferred.


================
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();
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147219