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

[Sergio Afonso via Phabricator via llvm-commits]

skatrak added inline comments.


================
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:
> > 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.
This is now implemented in semantics in patch D149337. I'm currently working on updating the rest of patches to stop using the `omp.atomic_default_mem_order` attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147219