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

Tue May 2 16:52:04 PDT 2023

kiranchandramohan 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);
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147219