[flang][OpenMP] Fix pointer variables in atomic read/write

[peixin]

Summary

For pointer variables, using getSymbolAddress cannot get the coorect
address for atomic read/write operands. Use genExprAddr to fix it.

[shraiysh]

Hi Peixin, thank you for this patch. I like the name change from numbered testcases to descriptive testcases, however can we please decouple that change from this change (as an NFC)? Right now it is hard to see the change in generated IR after this fix.

[peixin]

In D125793#3530154, @shraiysh wrote:

Hi Peixin, thank you for this patch. I like the name change from numbered testcases to descriptive testcases, however can we please decouple that change from this change (as an NFC)? Right now it is hard to see the change in generated IR after this fix.

Good point. Thanks for the notice. It is truly not easy to review when I look back to this patch now :). Will decouple the change. I should also add some comments/descriptions in the test case for what kind of IR it should generate with this pathc.

[peixin]

As @shraiysh suggested, decouple the previous patch and add some comments in the test cases to describe what IR should be generated after this patch.

[shraiysh]

flang/test/Lower/OpenMP/atomic-write.f90

49

Shouldn't this be the following?

%[[PTR_VAL:.*]] = fir.load %[[VAL_1]] : !fir.ref<!fir.ptr<i32>>
omp.atomic.write %[[PTR_VAL]] = %[[VAL_3]]   : !fir.ptr<i32>, i32

[peixin]

flang/test/Lower/OpenMP/atomic-write.f90

49

Right. I forgot to check tco. The previous fix fails to run tco. I had thought the codegen can handle this. Now fixed this.

[shraiysh]

@NimishMishra this LGTM, but as you are the author of the atomic read and write lowering, please check this patch. (adding you as a blocking reviewer)

[NimishMishra]

LGTM

flang/lib/Lower/OpenMP.cpp

 }
 
 static void
 genOmpAtomicWrite(Fortran::lower::AbstractConverter &converter,
                   Fortran::lower::pft::Evaluation &eval,
                   const Fortran::parser::OmpAtomicWrite &atomicWrite) {
   auto &firOpBuilder = converter.getFirOpBuilder();
   auto currentLocation = converter.getCurrentLocation();
-  mlir::Value address;
-  // If no hint clause is specified, the effect is as if
-  // hint(omp_sync_hint_none) had been specified.
-  mlir::IntegerAttr hint = nullptr;
-  mlir::omp::ClauseMemoryOrderKindAttr memory_order = nullptr;
+  // Get the value and address of atomic write operands.
   const Fortran::parser::OmpAtomicClauseList &rightHandClauseList =
       std::get<2>(atomicWrite.t);
   const Fortran::parser::OmpAtomicClauseList &leftHandClauseList =
       std::get<0>(atomicWrite.t);
   const auto &assignmentStmtExpr =
       std::get<Fortran::parser::Expr>(std::get<3>(atomicWrite.t).statement.t);
   const auto &assignmentStmtVariable = std::get<Fortran::parser::Variable>(
       std::get<3>(atomicWrite.t).statement.t);
   Fortran::lower::StatementContext stmtCtx;
-  auto value = fir::getBase(converter.genExprValue(
+  mlir::Value value = fir::getBase(converter.genExprValue(
       *Fortran::semantics::GetExpr(assignmentStmtExpr), stmtCtx));
-  if (auto varDesignator = std::get_if<
-          Fortran::common::Indirection<Fortran::parser::Designator>>(
-          &assignmentStmtVariable.u)) {
-    if (const auto *name = getDesignatorNameIfDataRef(varDesignator->value())) {
-      address = converter.getSymbolAddress(*name->symbol);
-    }
-  }
-
+  mlir::Value address = fir::getBase(converter.genExprAddr(
+      *Fortran::semantics::GetExpr(assignmentStmtVariable), stmtCtx));
+  // If no hint clause is specified, the effect is as if
+  // hint(omp_sync_hint_none) had been specified.
+  mlir::IntegerAttr hint = nullptr;
+  mlir::omp::ClauseMemoryOrderKindAttr memory_order = nullptr;
   genOmpAtomicHintAndMemoryOrderClauses(converter, leftHandClauseList, hint,
                                         memory_order);
   genOmpAtomicHintAndMemoryOrderClauses(converter, rightHandClauseList, hint,
                                         memory_order);
   firOpBuilder.create<mlir::omp::AtomicWriteOp>(currentLocation, address, value,
                                                 hint, memory_order);
 }
 
 static void genOmpAtomicRead(Fortran::lower::AbstractConverter &converter,
                              Fortran::lower::pft::Evaluation &eval,
                              const Fortran::parser::OmpAtomicRead &atomicRead) {
   auto &firOpBuilder = converter.getFirOpBuilder();
   auto currentLocation = converter.getCurrentLocation();
-  mlir::Value to_address;
-  mlir::Value from_address;
-  // If no hint clause is specified, the effect is as if
-  // hint(omp_sync_hint_none) had been specified.
-  mlir::IntegerAttr hint = nullptr;
-  mlir::omp::ClauseMemoryOrderKindAttr memory_order = nullptr;
+  // Get the address of atomic read operands.
   const Fortran::parser::OmpAtomicClauseList &rightHandClauseList =
       std::get<2>(atomicRead.t);
   const Fortran::parser::OmpAtomicClauseList &leftHandClauseList =
       std::get<0>(atomicRead.t);
   const auto &assignmentStmtExpr =
       std::get<Fortran::parser::Expr>(std::get<3>(atomicRead.t).statement.t);
   const auto &assignmentStmtVariable = std::get<Fortran::parser::Variable>(
       std::get<3>(atomicRead.t).statement.t);
-  if (auto exprDesignator = std::get_if<
-          Fortran::common::Indirection<Fortran::parser::Designator>>(
-          &assignmentStmtExpr.u)) {
-    if (const auto *name =
-            getDesignatorNameIfDataRef(exprDesignator->value())) {
-      from_address = converter.getSymbolAddress(*name->symbol);
-    }
-  }
-
-  if (auto varDesignator = std::get_if<
-          Fortran::common::Indirection<Fortran::parser::Designator>>(
-          &assignmentStmtVariable.u)) {
-    if (const auto *name = getDesignatorNameIfDataRef(varDesignator->value())) {
-      to_address = converter.getSymbolAddress(*name->symbol);
-    }
-  }
-
+  Fortran::lower::StatementContext stmtCtx;
+  mlir::Value from_address = fir::getBase(converter.genExprAddr(
+      *Fortran::semantics::GetExpr(assignmentStmtExpr), stmtCtx));
+  mlir::Value to_address = fir::getBase(converter.genExprAddr(
+      *Fortran::semantics::GetExpr(assignmentStmtVariable), stmtCtx));
+  // If no hint clause is specified, the effect is as if
+  // hint(omp_sync_hint_none) had been specified.
+  mlir::IntegerAttr hint = nullptr;
+  mlir::omp::ClauseMemoryOrderKindAttr memory_order = nullptr;
   genOmpAtomicHintAndMemoryOrderClauses(converter, leftHandClauseList, hint,
                                         memory_order);
   genOmpAtomicHintAndMemoryOrderClauses(converter, rightHandClauseList, hint,
                                         memory_order);
   firOpBuilder.create<mlir::omp::AtomicReadOp>(currentLocation, from_address,
                                                to_address, hint, memory_order);
 }
 

flang/test/Lower/OpenMP/atomic-read.f90

program OmpAtomic
     !$omp atomic read hint(omp_sync_hint_speculative)
        e = f
     !$omp atomic read hint(omp_sync_hint_nonspeculative)
        g = h
     !$omp atomic read
        g = h
 end program OmpAtomic
 
+! Test lowering atomic read for pointer variables.
+! Please notice to use %[[VAL_4]] and %[[VAL_1]] for operands of atomic
+! operation, instead of %[[VAL_3]] and %[[VAL_0]].
+
+!CHECK-LABEL: func.func @_QPatomic_read_pointer() {
+!CHECK:         %[[VAL_0:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "x", uniq_name = "_QFatomic_read_pointerEx"}
+!CHECK:         %[[VAL_1:.*]] = fir.alloca !fir.ptr<i32> {uniq_name = "_QFatomic_read_pointerEx.addr"}
+!CHECK:         %[[VAL_2:.*]] = fir.zero_bits !fir.ptr<i32>
+!CHECK:         fir.store %[[VAL_2]] to %[[VAL_1]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         %[[VAL_3:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "y", uniq_name = "_QFatomic_read_pointerEy"}
+!CHECK:         %[[VAL_4:.*]] = fir.alloca !fir.ptr<i32> {uniq_name = "_QFatomic_read_pointerEy.addr"}
+!CHECK:         %[[VAL_5:.*]] = fir.zero_bits !fir.ptr<i32>
+!CHECK:         fir.store %[[VAL_5]] to %[[VAL_4]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         %[[VAL_6:.*]] = fir.load %[[VAL_1]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         %[[VAL_7:.*]] = fir.load %[[VAL_4]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         omp.atomic.read %[[VAL_7]] = %[[VAL_6]]   : !fir.ptr<i32>
+!CHECK:         %[[VAL_8:.*]] = fir.load %[[VAL_4]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         %[[VAL_9:.*]] = fir.load %[[VAL_8]] : !fir.ptr<i32>
+!CHECK:         %[[VAL_10:.*]] = fir.load %[[VAL_1]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         fir.store %[[VAL_9]] to %[[VAL_10]] : !fir.ptr<i32>
+!CHECK:         return
+!CHECK:       }
+
+subroutine atomic_read_pointer()
+  integer, pointer :: x, y
+
+  !$omp atomic read
+    y = x
+
+  x = y
+end
+

flang/test/Lower/OpenMP/atomic-write.f90

program OmpAtomicWrite
 
     !$omp atomic write relaxed
         x = 7 * y
 
     !$omp atomic write release hint(omp_sync_hint_speculative)
         y = 10*x + z/2
 end program OmpAtomicWrite
 
+! Test lowering atomic read for pointer variables.
+! Please notice to use %[[VAL_1]] for operands of atomic operation, instead
+! of %[[VAL_0]].
+
+!CHECK-LABEL: func.func @_QPatomic_write_pointer() {
+!CHECK:         %[[VAL_0:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "x", uniq_name = "_QFatomic_write_pointerEx"}
+!CHECK:         %[[VAL_1:.*]] = fir.alloca !fir.ptr<i32> {uniq_name = "_QFatomic_write_pointerEx.addr"}
+!CHECK:         %[[VAL_2:.*]] = fir.zero_bits !fir.ptr<i32>
+!CHECK:         fir.store %[[VAL_2]] to %[[VAL_1]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         %[[VAL_3:.*]] = arith.constant 1 : i32
+!CHECK:         %[[VAL_4:.*]] = fir.load %[[VAL_1]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         omp.atomic.write %[[VAL_4]] = %[[VAL_3]]   : !fir.ptr<i32>, i32
+!CHECK:         %[[VAL_5:.*]] = arith.constant 2 : i32
+!CHECK:         %[[VAL_6:.*]] = fir.load %[[VAL_1]] : !fir.ref<!fir.ptr<i32>>
+!CHECK:         fir.store %[[VAL_5]] to %[[VAL_6]] : !fir.ptr<i32>
+!CHECK:         return
+!CHECK:       }
+
+subroutine atomic_write_pointer()
+  integer, pointer :: x
+
+  !$omp atomic write
+    x = 1
+
+  x = 2
+end
+