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

Tue May 2 16:29:13 PDT 2023

kiranchandramohan added a comment.

Thanks for the updates. Please see comments inline.

================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1080-1082
+  auto defaultAO = getAtomicDefaultMemOrder(opInst);
+  llvm::AtomicOrdering AO =
+      convertAtomicOrdering(readOp.getMemoryOrderVal(), defaultAO);
----------------
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.

================
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:
> > 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147219