Skip to content

Conversation

@jeanPerier
Copy link
Contributor

The semantic of pointer assignments inside FORALL requires evaluating the targets (RHS) and pointer variables (LHS) of all iterations before evaluating the assignments.

In practice, if the compiler can prove that the RHS and LHS evaluations are not impacted by the assignments, the evaluation of the FORALL assignment statement can be done in a single loop. However, if the compiler cannot prove this, it needs to "save" the addresses of the targets and/or the pointer descriptors of each iterations before doing the assignments.

This patch implement the most common cases where there is no lower bound spec, no bounds remapping, the LHS is not polymorphic, and the RHS is not NULL.

The HLFIR operation used to represent assignments inside FORALL can be used for pointer assignments to (the only difference being that the LHS is a descriptor address).

The analysis for intrinsic assignment can be reused, with the distinction that the RHS data is not read during the assignment.

The logic that is used to save LHS in intrinsic assignments inside FORALL is extracted to be used for the RHS of pointer assignments when needed (saving a descriptor value).
Pointer assignment LHS are just descriptor addresses and are saved as int_ptr values.

Note than pointer assignments inside FORALL do not seem to be vastly used outside of trivial cases given none gfortran, nvfortran, xlf, ifx, and nagfor pass the added Fortran test when ran end-to-end (some correctly deal with conflict with the RHS, but none is able to "save" the LHS correctly).

Remaining cases will be implemented in a separate patch.

The semantic of pointer assignments inside FORALL requires evaluating
the targets (RHS) and pointer variables (LHS) of all iterations before
evaluating the assignments.

In practice, if the compiler can prove that the RHS and LHS evaluations
are not impacted by the assignments, the evaluation of the FORALL
assignment statement can be done in a single loop.
However, if the compiler cannot prove this, it needs to "save" the addresses
of the targets and/or the pointer descriptors of each iterations before doing
the assignments.

This patch implement the most common cases where there is no lower bound spec,
no bounds remapping, the LHS is not polymorphic, and the RHS is not NULL.

The HLFIR operation used to represent assignments inside FORALL can be used
for pointer assignments to (the only difference being that the LHS is a
descriptor address).

The analysis for intrinsic assignment can be reused, with the distinction that
the RHS data is not read during the assignment.

The logic that is used to save LHS in intrinsic assignments inside FORALL is
extracted to be used for the RHS of pointer assignments when needed (saving
a descriptor value).
Pointer assignment LHS are just descriptor addresses and are saved as int_ptr
values.

Note than pointer assignments inside Forall do not seem to be vastly used
outside of trivial cases given none gfortran, nvfortran, xlf, ifx, and nagfor
pass the added Fortran test when ran end-to-end (some correctly deal with
conflict with the RHS, but none is able to "save" the LHS correctly).

Remaining cases will be implemented in a separate patch.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Mar 3, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 3, 2025

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

Author: None (jeanPerier)

Changes

The semantic of pointer assignments inside FORALL requires evaluating the targets (RHS) and pointer variables (LHS) of all iterations before evaluating the assignments.

In practice, if the compiler can prove that the RHS and LHS evaluations are not impacted by the assignments, the evaluation of the FORALL assignment statement can be done in a single loop. However, if the compiler cannot prove this, it needs to "save" the addresses of the targets and/or the pointer descriptors of each iterations before doing the assignments.

This patch implement the most common cases where there is no lower bound spec, no bounds remapping, the LHS is not polymorphic, and the RHS is not NULL.

The HLFIR operation used to represent assignments inside FORALL can be used for pointer assignments to (the only difference being that the LHS is a descriptor address).

The analysis for intrinsic assignment can be reused, with the distinction that the RHS data is not read during the assignment.

The logic that is used to save LHS in intrinsic assignments inside FORALL is extracted to be used for the RHS of pointer assignments when needed (saving a descriptor value).
Pointer assignment LHS are just descriptor addresses and are saved as int_ptr values.

Note than pointer assignments inside FORALL do not seem to be vastly used outside of trivial cases given none gfortran, nvfortran, xlf, ifx, and nagfor pass the added Fortran test when ran end-to-end (some correctly deal with conflict with the RHS, but none is able to "save" the LHS correctly).

Remaining cases will be implemented in a separate patch.


Patch is 43.24 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/129522.diff

13 Files Affected:

  • (modified) flang/include/flang/Optimizer/Builder/HLFIRTools.h (+4-1)
  • (modified) flang/include/flang/Optimizer/Builder/TemporaryStorage.h (+19-1)
  • (modified) flang/include/flang/Optimizer/Dialect/FIRType.h (+3)
  • (modified) flang/include/flang/Optimizer/HLFIR/HLFIROps.td (+1-1)
  • (modified) flang/lib/Lower/Bridge.cpp (+62-3)
  • (modified) flang/lib/Optimizer/Builder/HLFIRTools.cpp (+33-5)
  • (modified) flang/lib/Optimizer/Builder/TemporaryStorage.cpp (+24)
  • (modified) flang/lib/Optimizer/Dialect/FIRType.cpp (+4)
  • (modified) flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp (+14)
  • (modified) flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp (+80-24)
  • (modified) flang/lib/Optimizer/HLFIR/Transforms/ScheduleOrderedAssignments.cpp (+6-3)
  • (added) flang/test/HLFIR/order_assignments/forall-pointer-assignment-codegen.fir (+200)
  • (added) flang/test/HLFIR/order_assignments/forall-pointer-assignment-scheduling.f90 (+111)
diff --git a/flang/include/flang/Optimizer/Builder/HLFIRTools.h b/flang/include/flang/Optimizer/Builder/HLFIRTools.h
index 8b1235b50cc6f..951050b135985 100644
--- a/flang/include/flang/Optimizer/Builder/HLFIRTools.h
+++ b/flang/include/flang/Optimizer/Builder/HLFIRTools.h
@@ -249,8 +249,11 @@ mlir::Value genVariableBoxChar(mlir::Location loc, fir::FirOpBuilder &builder,
                                hlfir::Entity var);
 
 /// Get or create a fir.box or fir.class from a variable.
+/// A fir.box with different attributes that \p var can be created
+/// using \p forceBoxType.
 hlfir::Entity genVariableBox(mlir::Location loc, fir::FirOpBuilder &builder,
-                             hlfir::Entity var);
+                             hlfir::Entity var,
+                             fir::BaseBoxType forceBoxType = {});
 
 /// If the entity is a variable, load its value (dereference pointers and
 /// allocatables if needed). Do nothing if the entity is already a value, and
diff --git a/flang/include/flang/Optimizer/Builder/TemporaryStorage.h b/flang/include/flang/Optimizer/Builder/TemporaryStorage.h
index 5f2e1c4b510b0..b17a75354e7d1 100644
--- a/flang/include/flang/Optimizer/Builder/TemporaryStorage.h
+++ b/flang/include/flang/Optimizer/Builder/TemporaryStorage.h
@@ -179,6 +179,8 @@ class AnyValueStack {
 /// type. Fetching variable N will return a variable with the same address,
 /// dynamic type, bounds, and type parameters as the Nth variable that was
 /// pushed. It is implemented using runtime.
+/// Note that this is not meant to save POINTER or ALLOCATABLE descriptor
+/// addresses, use AnyDescriptorAddressStack instead.
 class AnyVariableStack {
 public:
   AnyVariableStack(mlir::Location loc, fir::FirOpBuilder &builder,
@@ -203,6 +205,21 @@ class AnyVariableStack {
   mlir::Value retValueBox;
 };
 
+/// Data structure to stack descriptor addresses. It stores the descriptor
+/// addresses as int_ptr values under the hood.
+class AnyDescriptorAddressStack : public AnyValueStack {
+public:
+  AnyDescriptorAddressStack(mlir::Location loc, fir::FirOpBuilder &builder,
+                            mlir::Type descriptorAddressType);
+
+  void pushValue(mlir::Location loc, fir::FirOpBuilder &builder,
+                 mlir::Value value);
+  mlir::Value fetch(mlir::Location loc, fir::FirOpBuilder &builder);
+
+private:
+  mlir::Type descriptorAddressType;
+};
+
 class TemporaryStorage;
 
 /// Data structure to stack vector subscripted entity shape and
@@ -264,7 +281,8 @@ class TemporaryStorage {
 
 private:
   std::variant<HomogeneousScalarStack, SimpleCopy, SSARegister, AnyValueStack,
-               AnyVariableStack, AnyVectorSubscriptStack>
+               AnyVariableStack, AnyVectorSubscriptStack,
+               AnyDescriptorAddressStack>
       impl;
 };
 } // namespace fir::factory
diff --git a/flang/include/flang/Optimizer/Dialect/FIRType.h b/flang/include/flang/Optimizer/Dialect/FIRType.h
index 1e637895d8e99..3d30f4e673682 100644
--- a/flang/include/flang/Optimizer/Dialect/FIRType.h
+++ b/flang/include/flang/Optimizer/Dialect/FIRType.h
@@ -53,6 +53,9 @@ class BaseBoxType : public mlir::Type {
   /// Is this the box for an assumed rank?
   bool isAssumedRank() const;
 
+  /// Is this a box for a pointer?
+  bool isPointer() const;
+
   /// Return the same type, except for the shape, that is taken the shape
   /// of shapeMold.
   BaseBoxType getBoxTypeWithNewShape(mlir::Type shapeMold) const;
diff --git a/flang/include/flang/Optimizer/HLFIR/HLFIROps.td b/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
index f4102538efc3c..c12066b1346f6 100644
--- a/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
+++ b/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
@@ -1377,7 +1377,7 @@ def hlfir_RegionAssignOp : hlfir_Op<"region_assign", [hlfir_OrderedAssignmentTre
         regions.push_back(&getUserDefinedAssignment());
     }
     mlir::Region* getSubTreeRegion() { return nullptr; }
-
+    bool isPointerAssignment();
   }];
 
   let hasCustomAssemblyFormat = 1;
diff --git a/flang/lib/Lower/Bridge.cpp b/flang/lib/Lower/Bridge.cpp
index cc19f335cd017..4c6e47d250329 100644
--- a/flang/lib/Lower/Bridge.cpp
+++ b/flang/lib/Lower/Bridge.cpp
@@ -4355,6 +4355,62 @@ class FirConverter : public Fortran::lower::AbstractConverter {
                                         stmtCtx);
   }
 
+  void genForallPointerAssignment(
+      mlir::Location loc, const Fortran::evaluate::Assignment &assign,
+      const Fortran::evaluate::Assignment::BoundsSpec &lbExprs) {
+    if (Fortran::evaluate::IsProcedureDesignator(assign.rhs))
+      TODO(loc, "procedure pointer assignment inside FORALL");
+    std::optional<Fortran::evaluate::DynamicType> lhsType =
+        assign.lhs.GetType();
+    // Polymorphic pointer assignment is delegated to the runtime, and
+    // PointerAssociateLowerBounds needs the lower bounds as arguments, so they
+    // must be preserved.
+    if (lhsType && lhsType->IsPolymorphic())
+      TODO(loc, "polymorphic pointer assignment in FORALL");
+    // Nullification is special, there is no RHS that can be prepared,
+    // need to encode it in HLFIR.
+    if (Fortran::evaluate::UnwrapExpr<Fortran::evaluate::NullPointer>(
+            assign.rhs))
+      TODO(loc, "NULL pointer assignment in FORALL");
+    // Lower bounds could be "applied" when preparing RHS, but in order
+    // to deal with the polymorphic case and to reuse existing pointer
+    // assignment helpers in HLFIR codegen, it is better to keep them
+    // separate.
+    if (!lbExprs.empty())
+      TODO(loc, "Pointer assignment with new lower bounds inside FORALL");
+    // Otherwise, this is a "dumb" pointer assignment that can be represented
+    // with hlfir.region_assign with descriptor address/value and later
+    // implemented with a store.
+    auto regionAssignOp = builder->create<hlfir::RegionAssignOp>(loc);
+
+    // Lower LHS in its own region.
+    builder->createBlock(&regionAssignOp.getLhsRegion());
+    Fortran::lower::StatementContext lhsContext;
+    hlfir::Entity lhs = Fortran::lower::convertExprToHLFIR(
+        loc, *this, assign.lhs, localSymbols, lhsContext);
+
+    auto lhsYieldOp = builder->create<hlfir::YieldOp>(loc, lhs);
+    Fortran::lower::genCleanUpInRegionIfAny(
+        loc, *builder, lhsYieldOp.getCleanup(), lhsContext);
+
+    // Lower RHS in its own region.
+    builder->createBlock(&regionAssignOp.getRhsRegion());
+    Fortran::lower::StatementContext rhsContext;
+    hlfir::Entity rhs = Fortran::lower::convertExprToHLFIR(
+        loc, *this, assign.rhs, localSymbols, rhsContext);
+    // Create pointer descriptor value from the RHS.
+    if (rhs.isMutableBox())
+      rhs = hlfir::Entity{builder->create<fir::LoadOp>(loc, rhs)};
+    auto lhsBoxType =
+        llvm::cast<fir::BaseBoxType>(fir::unwrapRefType(lhs.getType()));
+    mlir::Value newBox = hlfir::genVariableBox(loc, *builder, rhs, lhsBoxType);
+    auto rhsYieldOp = builder->create<hlfir::YieldOp>(loc, newBox);
+    Fortran::lower::genCleanUpInRegionIfAny(
+        loc, *builder, rhsYieldOp.getCleanup(), rhsContext);
+
+    builder->setInsertionPointAfter(regionAssignOp);
+  }
+
   // Create the 2 x newRank array with the bounds to be passed to the runtime as
   // a descriptor.
   mlir::Value createBoundArray(llvm::ArrayRef<mlir::Value> lbounds,
@@ -4793,13 +4849,16 @@ class FirConverter : public Fortran::lower::AbstractConverter {
               },
               [&](const Fortran::evaluate::Assignment::BoundsSpec &lbExprs) {
                 if (isInsideHlfirForallOrWhere())
-                  TODO(loc, "pointer assignment inside FORALL");
-                genPointerAssignment(loc, assign, lbExprs);
+                  genForallPointerAssignment(loc, assign, lbExprs);
+                else
+                  genPointerAssignment(loc, assign, lbExprs);
               },
               [&](const Fortran::evaluate::Assignment::BoundsRemapping
                       &boundExprs) {
                 if (isInsideHlfirForallOrWhere())
-                  TODO(loc, "pointer assignment inside FORALL");
+                  TODO(
+                      loc,
+                      "pointer assignment with bounds remapping inside FORALL");
                 genPointerAssignment(loc, assign, boundExprs);
               },
           },
diff --git a/flang/lib/Optimizer/Builder/HLFIRTools.cpp b/flang/lib/Optimizer/Builder/HLFIRTools.cpp
index 8993065c2bb64..c5a7205afb796 100644
--- a/flang/lib/Optimizer/Builder/HLFIRTools.cpp
+++ b/flang/lib/Optimizer/Builder/HLFIRTools.cpp
@@ -349,26 +349,54 @@ mlir::Value hlfir::genVariableBoxChar(mlir::Location loc,
                                           lengths[0]);
 }
 
+static hlfir::Entity changeBoxAttributes(mlir::Location loc,
+                                         fir::FirOpBuilder &builder,
+                                         hlfir::Entity var,
+                                         fir::BaseBoxType forceBoxType) {
+  assert(llvm::isa<fir::BaseBoxType>(var.getType()) && "expect box type");
+  // Propagate lower bounds.
+  mlir::Value shift;
+  llvm::SmallVector<mlir::Value> lbounds =
+      getNonDefaultLowerBounds(loc, builder, var);
+  if (!lbounds.empty())
+    shift = builder.genShift(loc, lbounds);
+  auto rebox = builder.create<fir::ReboxOp>(loc, forceBoxType, var, shift,
+                                            /*slice=*/nullptr);
+  return hlfir::Entity{rebox};
+}
+
 hlfir::Entity hlfir::genVariableBox(mlir::Location loc,
                                     fir::FirOpBuilder &builder,
-                                    hlfir::Entity var) {
+                                    hlfir::Entity var,
+                                    fir::BaseBoxType forceBoxType) {
   assert(var.isVariable() && "must be a variable");
   var = hlfir::derefPointersAndAllocatables(loc, builder, var);
-  if (mlir::isa<fir::BaseBoxType>(var.getType()))
-    return var;
+  if (mlir::isa<fir::BaseBoxType>(var.getType())) {
+    if (!forceBoxType || forceBoxType == var.getType())
+      return var;
+    return changeBoxAttributes(loc, builder, var, forceBoxType);
+  }
   // Note: if the var is not a fir.box/fir.class at that point, it has default
   // lower bounds and is not polymorphic.
   mlir::Value shape =
       var.isArray() ? hlfir::genShape(loc, builder, var) : mlir::Value{};
   llvm::SmallVector<mlir::Value> typeParams;
-  auto maybeCharType =
-      mlir::dyn_cast<fir::CharacterType>(var.getFortranElementType());
+  mlir::Type elementType =
+      forceBoxType ? fir::getFortranElementType(forceBoxType.getEleTy())
+                   : var.getFortranElementType();
+  auto maybeCharType = mlir::dyn_cast<fir::CharacterType>(elementType);
   if (!maybeCharType || maybeCharType.hasDynamicLen())
     hlfir::genLengthParameters(loc, builder, var, typeParams);
   mlir::Value addr = var.getBase();
   if (mlir::isa<fir::BoxCharType>(var.getType()))
     addr = genVariableRawAddress(loc, builder, var);
   mlir::Type boxType = fir::BoxType::get(var.getElementOrSequenceType());
+  if (forceBoxType) {
+    boxType = forceBoxType;
+    mlir::Type baseType =
+        fir::ReferenceType::get(fir::unwrapRefType(forceBoxType.getEleTy()));
+    addr = builder.createConvert(loc, baseType, addr);
+  }
   auto embox =
       builder.create<fir::EmboxOp>(loc, boxType, addr, shape,
                                    /*slice=*/mlir::Value{}, typeParams);
diff --git a/flang/lib/Optimizer/Builder/TemporaryStorage.cpp b/flang/lib/Optimizer/Builder/TemporaryStorage.cpp
index 4c59574dd433a..48c2cb2181a0b 100644
--- a/flang/lib/Optimizer/Builder/TemporaryStorage.cpp
+++ b/flang/lib/Optimizer/Builder/TemporaryStorage.cpp
@@ -355,3 +355,27 @@ void fir::factory::AnyVectorSubscriptStack::destroy(
   static_cast<AnyVariableStack *>(this)->destroy(loc, builder);
   shapeTemp->destroy(loc, builder);
 }
+
+//===----------------------------------------------------------------------===//
+// fir::factory::AnyDescriptorAddressStack implementation.
+//===----------------------------------------------------------------------===//
+
+fir::factory::AnyDescriptorAddressStack::AnyDescriptorAddressStack(
+    mlir::Location loc, fir::FirOpBuilder &builder,
+    mlir::Type descriptorAddressType)
+    : AnyValueStack(loc, builder, builder.getIntPtrType()),
+      descriptorAddressType{descriptorAddressType} {}
+
+void fir::factory::AnyDescriptorAddressStack::pushValue(
+    mlir::Location loc, fir::FirOpBuilder &builder, mlir::Value variable) {
+  mlir::Value cast =
+      builder.createConvert(loc, builder.getIntPtrType(), variable);
+  static_cast<AnyValueStack *>(this)->pushValue(loc, builder, cast);
+}
+
+mlir::Value
+fir::factory::AnyDescriptorAddressStack::fetch(mlir::Location loc,
+                                               fir::FirOpBuilder &builder) {
+  mlir::Value addr = static_cast<AnyValueStack *>(this)->fetch(loc, builder);
+  return builder.createConvert(loc, descriptorAddressType, addr);
+}
diff --git a/flang/lib/Optimizer/Dialect/FIRType.cpp b/flang/lib/Optimizer/Dialect/FIRType.cpp
index 719cb1b9d75aa..1277b50fa3b29 100644
--- a/flang/lib/Optimizer/Dialect/FIRType.cpp
+++ b/flang/lib/Optimizer/Dialect/FIRType.cpp
@@ -1358,6 +1358,10 @@ bool fir::BaseBoxType::isAssumedRank() const {
   return false;
 }
 
+bool fir::BaseBoxType::isPointer() const {
+  return llvm::isa<fir::PointerType>(getEleTy());
+}
+
 //===----------------------------------------------------------------------===//
 // FIROpsDialect
 //===----------------------------------------------------------------------===//
diff --git a/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp b/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
index 2fcfa1353f86b..383e6a2630537 100644
--- a/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
+++ b/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
@@ -1891,6 +1891,20 @@ llvm::LogicalResult hlfir::RegionAssignOp::verify() {
   return mlir::success();
 }
 
+bool hlfir::RegionAssignOp::isPointerAssignment() {
+  if (!getUserDefinedAssignment().empty())
+    return false;
+  hlfir::YieldOp yieldOp =
+      mlir::dyn_cast_or_null<hlfir::YieldOp>(getTerminator(getLhsRegion()));
+  if (!yieldOp)
+    return false;
+  mlir::Type lhsType = yieldOp.getEntity().getType();
+  if (!hlfir::isBoxAddressType(lhsType))
+    return false;
+  auto baseBoxType = llvm::cast<fir::BaseBoxType>(fir::unwrapRefType(lhsType));
+  return baseBoxType.isPointer();
+}
+
 //===----------------------------------------------------------------------===//
 // YieldOp
 //===----------------------------------------------------------------------===//
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp b/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp
index cba1bfc74e922..5911458998af0 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/LowerHLFIROrderedAssignments.cpp
@@ -224,6 +224,10 @@ class OrderedAssignmentRewriter {
   /// Save a value for subsequent runs.
   void generateSaveEntity(hlfir::SaveEntity savedEntity,
                           bool willUseSavedEntityInSameRun);
+  /// Save a variable address instead of its value.
+  void saveNonVectorSubscriptedAddress(hlfir::SaveEntity savedEntity);
+  /// Save a LHS variable address instead of its value, handling the cases
+  /// where the LHS is vector subscripted.
   void saveLeftHandSide(hlfir::SaveEntity savedEntity,
                         hlfir::RegionAssignOp regionAssignOp);
 
@@ -444,7 +448,16 @@ convertToMoldType(mlir::Location loc, fir::FirOpBuilder &builder,
 
 void OrderedAssignmentRewriter::pre(hlfir::RegionAssignOp regionAssignOp) {
   mlir::Location loc = regionAssignOp.getLoc();
-  std::optional<hlfir::LoopNest> elementalLoopNest;
+  if (regionAssignOp.isPointerAssignment()) {
+    auto [lhsValue, oldLhsYield] =
+        generateYieldedEntity(regionAssignOp.getLhsRegion());
+    auto [rhsValue, oldRhsYield] =
+        generateYieldedEntity(regionAssignOp.getRhsRegion());
+    builder.createStoreWithConvert(loc, rhsValue, lhsValue);
+    generateCleanupIfAny(oldLhsYield);
+    generateCleanupIfAny(oldRhsYield);
+    return;
+  }
   auto [rhsValue, oldRhsYield] =
       generateYieldedEntity(regionAssignOp.getRhsRegion());
   hlfir::Entity rhsEntity{rhsValue};
@@ -1075,6 +1088,12 @@ getAssignIfLeftHandSideRegion(mlir::Region &region) {
   return nullptr;
 }
 
+static bool isPointerAssignmentRHS(mlir::Region &region) {
+  auto assign = mlir::dyn_cast<hlfir::RegionAssignOp>(region.getParentOp());
+  return assign && assign.isPointerAssignment() &&
+         (&assign.getRhsRegion() == &region);
+}
+
 bool OrderedAssignmentRewriter::currentLoopNestIterationNumberCanBeComputed(
     llvm::SmallVectorImpl<fir::DoLoopOp> &loopNest) {
   if (constructStack.empty())
@@ -1139,6 +1158,11 @@ void OrderedAssignmentRewriter::generateSaveEntity(
            "lhs cannot be used in the loop nest where it is saved");
     return saveLeftHandSide(savedEntity, regionAssignOp);
   }
+  if (isPointerAssignmentRHS(region)) {
+    assert(!willUseSavedEntityInSameRun &&
+           "rhs cannot be used in the loop nest where it is saved");
+    return saveNonVectorSubscriptedAddress(savedEntity);
+  }
 
   mlir::Location loc = region.getParentOp()->getLoc();
   // Evaluate the region inside the loop nest (if any).
@@ -1230,15 +1254,56 @@ static bool rhsIsArray(hlfir::RegionAssignOp regionAssignOp) {
   return yieldOp && hlfir::Entity{yieldOp.getEntity()}.isArray();
 }
 
+static bool isVectorSubscripted(mlir::Region &region) {
+  return llvm::isa<hlfir::ElementalAddrOp>(region.back().back());
+}
+
+void OrderedAssignmentRewriter::saveNonVectorSubscriptedAddress(
+    hlfir::SaveEntity savedEntity) {
+  mlir::Region &region = *savedEntity.yieldRegion;
+  mlir::Location loc = region.getParentOp()->getLoc();
+  assert(!isVectorSubscripted(region) &&
+         "expected variable without vector subscripts");
+  ValueAndCleanUp varAndCleanup = generateYieldedEntity(region);
+  hlfir::Entity var{varAndCleanup.first};
+  fir::factory::TemporaryStorage *temp = nullptr;
+  // If the address dominates the constructs, its SSA value can simply be
+  // tracked and there is no need to save the address in memory.  Otherwise,
+  // the addresses are stored at each iteration in memory with a descriptor
+  // stack.
+  if (constructStack.empty() ||
+      dominanceInfo.properlyDominates(var, constructStack[0]))
+    doBeforeLoopNest(
+        [&] { temp = insertSavedEntity(region, fir::factory::SSARegister{}); });
+  else
+    doBeforeLoopNest([&] {
+      if (var.isMutableBox())
+        temp =
+            insertSavedEntity(region, fir::factory::AnyDescriptorAddressStack{
+                                          loc, builder, var.getType()});
+      else
+        temp = insertSavedEntity(region, fir::factory::AnyVariableStack{
+                                             loc, builder, var.getType()});
+    });
+  temp->pushValue(loc, builder, var);
+  generateCleanupIfAny(varAndCleanup.second);
+}
+
 void OrderedAssignmentRewriter::saveLeftHandSide(
     hlfir::SaveEntity savedEntity, hlfir::RegionAssignOp regionAssignOp) {
   mlir::Region &region = *savedEntity.yieldRegion;
+  if (!isVectorSubscripted(region)) {
+    saveNonVectorSubscriptedAddress(savedEntity);
+    return;
+  }
+  // Save vector subscripted LHS address.
   mlir::Location loc = region.getParentOp()->getLoc();
   LhsValueAndCleanUp loweredLhs = generateYieldedLHS(loc, region);
+  assert(loweredLhs.vectorSubscriptLoopNest &&
+         "expect vector subscript loop nest");
+  constructStack.push_back(loweredLhs.vectorSubscriptLoopNest->outerOp);
   fir::factory::TemporaryStorage *temp = nullptr;
-  if (loweredLhs.vectorSubscriptLoopNest)
-    constructStack.push_back(loweredLhs.vectorSubscriptLoopNest->outerOp);
-  if (loweredLhs.vectorSubscriptLoopNest && !rhsIsArray(regionAssignOp)) {
+  if (!rhsIsArray(regionAssignOp)) {
     // Vector subscripted entity for which the shape must also be saved on top
     // of the element addresses (e.g. the shape may change in each forall
     // iteration and is needed to create the elemental loops).
@@ -1264,29 +1329,20 @@ void OrderedAssignmentRewriter::saveLeftHandSide(
     vectorTmp.pushShape(loc, builder, shape);
     builder.restoreInsertionPoint(insertionPoint);
   } else {
-    // Otherwise, only save the LHS address.
-    //...
[truncated]

@jeanPerier
Copy link
Contributor Author

jeanPerier commented Mar 3, 2025

@klausler, @eugeneepshteyn, and @akuhlens, I added you as reviewers to validate my expectations in the added forall-pointer-assignment-scheduling.f90 test.

As mentioned in the commit, none of the compilers I tested generate an executable that prints 1 1 1 1 1 1 1 1 1 1 for test_need_to_save_lhs test (most print 1 1 1 1 1 5 4 3 2 1 which indicates they did not evaluate/save all the LHS descriptor addresses before doing the pointer assignments). To me this is an obvious violation of F2023 10.2.4.2.4 point 3, but I am surprised even NAG is not producing the result I expect here.

Copy link
Contributor

@vzakhari vzakhari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@jeanPerier jeanPerier merged commit 7302e1b into main Mar 5, 2025
11 checks passed
@jeanPerier jeanPerier deleted the users/jeanPerier/pointer-forall branch March 5, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flang:fir-hlfir flang Flang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants