[flang][OpenMP] Improve handling of REQUIRES ATOMIC_DEFAULT_MEM_ORDER

[kparzysz]

According to OpenMP 5.0 rules, the ACQ_REL ordering coming from a REQUIRES directive may need to be replaced with ACQUIRE or RELEASE depending on the directive in the ATOMIC construct. This was not done, leading to an incorrect "memory-order" clause appearing in the generated HLFIR.

This may need to be relaxed a bit to fully comply with later spec versions, that will be done in a future PR.

[llvmbot]

@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-semantics

Author: Krzysztof Parzyszek (kparzysz)

Changes

According to OpenMP 5.0 rules, the ACQ_REL ordering coming from a REQUIRES directive may need to be replaced with ACQUIRE or RELEASE depending on the directive in the ATOMIC construct. This was not done, leading to an incorrect "memory-order" clause appearing in the generated HLFIR.

This may need to be relaxed a bit to fully comply with later spec versions, that will be done in a future PR.

---

Full diff: https://github.com/llvm/llvm-project/pull/143917.diff

3 Files Affected:

• (modified) flang/lib/Semantics/rewrite-directives.cpp (+14-1)
• (added) flang/test/Lower/OpenMP/requires-atomic-default-mem-order.f90 (+22)
• (modified) flang/test/Semantics/OpenMP/requires-atomic02.f90 (+4-4)

diff --git a/flang/lib/Semantics/rewrite-directives.cpp b/flang/lib/Semantics/rewrite-directives.cpp
index b4fef2c881b67..91b60ea151dee 100644
--- a/flang/lib/Semantics/rewrite-directives.cpp
+++ b/flang/lib/Semantics/rewrite-directives.cpp
@@ -112,9 +112,22 @@ bool OmpRewriteMutator::Pre(parser::OpenMPAtomicConstruct &x) {
 
   // Add a memory order clause to the atomic directive.
   atomicDirectiveDefaultOrderFound_ = true;
+  llvm::omp::Clause kind{x.GetKind()};
   switch (*defaultMemOrder) {
   case common::OmpMemoryOrderType::Acq_Rel:
-    clauseList->v.emplace_back(parser::OmpClause{parser::OmpClause::AcqRel{}});
+    // FIXME: Implement 5.0 rules, pending clarification on later spec
+    // versions.
+    // [5.0:62:22-26]
+    if (kind == llvm::omp::Clause::OMPC_read) {
+      clauseList->v.emplace_back(
+          parser::OmpClause{parser::OmpClause::Acquire{}});
+    } else if (kind == llvm::omp::Clause::OMPC_update && x.IsCapture()) {
+      clauseList->v.emplace_back(
+          parser::OmpClause{parser::OmpClause::AcqRel{}});
+    } else {
+      clauseList->v.emplace_back(
+          parser::OmpClause{parser::OmpClause::Release{}});
+    }
     break;
   case common::OmpMemoryOrderType::Relaxed:
     clauseList->v.emplace_back(parser::OmpClause{parser::OmpClause::Relaxed{}});
diff --git a/flang/test/Lower/OpenMP/requires-atomic-default-mem-order.f90 b/flang/test/Lower/OpenMP/requires-atomic-default-mem-order.f90
new file mode 100644
index 0000000000000..91cb654aeeb3a
--- /dev/null
+++ b/flang/test/Lower/OpenMP/requires-atomic-default-mem-order.f90
@@ -0,0 +1,22 @@
+!RUN: %flang_fc1 -emit-hlfir %openmp_flags -fopenmp-version=50 %s -o - | FileCheck %s
+
+module m
+!$omp requires atomic_default_mem_order(acq_rel)
+
+contains
+
+!CHECK: %[[V:[0-9]+]]:2 = hlfir.declare {{.*}} {uniq_name = "_QMmFf00Ev"}
+!CHECK: %[[X:[0-9]+]]:2 = hlfir.declare {{.*}} {uniq_name = "_QMmFf00Ex"}
+!CHECK: omp.atomic.read %[[V]]#0 = %[[X]]#0 memory_order(acquire)
+!CHECK: omp.atomic.write %[[X]]#0 = %{{[0-9]+}} memory_order(release)
+
+subroutine f00(x, v)
+  integer :: x, v
+  !$omp atomic read
+    v = x
+
+  !$omp atomic write
+    x = v
+end
+
+end module
diff --git a/flang/test/Semantics/OpenMP/requires-atomic02.f90 b/flang/test/Semantics/OpenMP/requires-atomic02.f90
index a3724a83456fd..04a9b7a09aa98 100644
--- a/flang/test/Semantics/OpenMP/requires-atomic02.f90
+++ b/flang/test/Semantics/OpenMP/requires-atomic02.f90
@@ -12,7 +12,7 @@ program requires
 
   ! CHECK-LABEL: OpenMPAtomicConstruct
   ! CHECK: OmpClause -> Read
-  ! CHECK: OmpClause -> AcqRel
+  ! CHECK: OmpClause -> Acquire
   !$omp atomic read
   i = j
 
@@ -36,7 +36,7 @@ program requires
 
   ! CHECK-LABEL: OpenMPAtomicConstruct
   ! CHECK: OmpClause -> Write
-  ! CHECK: OmpClause -> AcqRel
+  ! CHECK: OmpClause -> Release
   !$omp atomic write
   i = j
 
@@ -60,7 +60,7 @@ program requires
 
   ! CHECK-LABEL: OpenMPAtomicConstruct
   ! CHECK: OmpClause -> Update
-  ! CHECK: OmpClause -> AcqRel
+  ! CHECK: OmpClause -> Release
   !$omp atomic update
   i = i + j
 
@@ -79,7 +79,7 @@ program requires
   i = i + j
 
   ! CHECK-LABEL: OpenMPAtomicConstruct
-  ! CHECK: OmpClause -> AcqRel
+  ! CHECK: OmpClause -> Release
   !$omp atomic
   i = i + j
 

[tblah]

Could this lead to unparsed sources not matching what was written in the original sources? I would implement something like this in MLIR or MLIR->LLVMIR conversion because the decision is internal for flang.

That said, I'm not sure what exactly the support expectations for unparsing. It is probably just a debugging tool for people working on flang, in which case this is fine.

[kparzysz]

I agree that handling of this part of the REQUIRES directive would be better placed somewhere else, but it would require rewriting this code.
We're hitting a failure in an internal build caused by this, so I'm hoping to get that fixed soon. After that I'd be happy to implement a better fix.