Skip to content

[flang] Fix optimization of array assignments after #146408 #147371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 8, 2025

Conversation

luporl
Copy link
Contributor

@luporl luporl commented Jul 7, 2025

Host associated variables were not being handled properly.
For array references, get the fixed shape extents from the value
type instead, that works correctly in all cases.

Host associated variables were not being handled properly.
For array references, get the fixed shape extents from the value
type instead, that works correctly in all cases.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Jul 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 7, 2025

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

Author: Leandro Lupori (luporl)

Changes

Host associated variables were not being handled properly.
For array references, get the fixed shape extents from the value
type instead, that works correctly in all cases.


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

2 Files Affected:

  • (modified) flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp (+5-6)
  • (modified) flang/test/HLFIR/opt-scalar-assign.fir (+29)
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp b/flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp
index 54892ef99bf58..434973f5887e8 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/OptimizedBufferization.cpp
@@ -805,13 +805,12 @@ llvm::LogicalResult BroadcastAssignBufferization::matchAndRewrite(
                                                shape, /*slice=*/mlir::Value{});
     } else {
       // Array references must have fixed shape, when used in assignments.
+      auto refTy = mlir::cast<fir::ReferenceType>(lhs.getType());
+      auto seqTy = mlir::cast<fir::SequenceType>(refTy.getElementType());
+      llvm::ArrayRef<int64_t> fixedShape = seqTy.getShape();
       int64_t flatExtent = 1;
-      for (const mlir::Value &extent : extents) {
-        mlir::Operation *op = extent.getDefiningOp();
-        assert(op && "no defining operation for constant array extent");
-        flatExtent *= fir::toInt(mlir::cast<mlir::arith::ConstantOp>(*op));
-      }
-
+      for (int64_t extent : fixedShape)
+        flatExtent *= extent;
       flatArrayType =
           fir::ReferenceType::get(fir::SequenceType::get({flatExtent}, eleTy));
       flatArray = builder.createConvert(loc, flatArrayType, flatArray);
diff --git a/flang/test/HLFIR/opt-scalar-assign.fir b/flang/test/HLFIR/opt-scalar-assign.fir
index 74cdcd9622adb..468a5dbf988d7 100644
--- a/flang/test/HLFIR/opt-scalar-assign.fir
+++ b/flang/test/HLFIR/opt-scalar-assign.fir
@@ -155,3 +155,32 @@ func.func @_QPtest6(%arg0: !fir.ref<!fir.box<!fir.heap<!fir.array<?x?xi32>>>> {f
 // CHECK:           }
 // CHECK:           return
 // CHECK:         }
+
+func.func @_QQmain() {
+  return
+}
+
+func.func private @_QFPtest7(%arg0: !fir.ref<tuple<!fir.box<!fir.array<2x2xi32>>>> {fir.host_assoc}) attributes {fir.host_symbol = @_QQmain, llvm.linkage = #llvm.linkage<internal>} {
+  %0 = fir.dummy_scope : !fir.dscope
+  %c0_i32 = arith.constant 0 : i32
+  %1 = fir.coordinate_of %arg0, %c0_i32 : (!fir.ref<tuple<!fir.box<!fir.array<2x2xi32>>>>, i32) -> !fir.ref<!fir.box<!fir.array<2x2xi32>>>
+  %2 = fir.load %1 : !fir.ref<!fir.box<!fir.array<2x2xi32>>>
+  %3 = fir.box_addr %2 : (!fir.box<!fir.array<2x2xi32>>) -> !fir.ref<!fir.array<2x2xi32>>
+  %c0 = arith.constant 0 : index
+  %4:3 = fir.box_dims %2, %c0 : (!fir.box<!fir.array<2x2xi32>>, index) -> (index, index, index)
+  %c1 = arith.constant 1 : index
+  %5:3 = fir.box_dims %2, %c1 : (!fir.box<!fir.array<2x2xi32>>, index) -> (index, index, index)
+  %6 = fir.shape %4#1, %5#1 : (index, index) -> !fir.shape<2>
+  %7:2 = hlfir.declare %3(%6) {fortran_attrs = #fir.var_attrs<host_assoc>, uniq_name = "_QFEa"} : (!fir.ref<!fir.array<2x2xi32>>, !fir.shape<2>) -> (!fir.ref<!fir.array<2x2xi32>>, !fir.ref<!fir.array<2x2xi32>>)
+  %c0_i32_0 = arith.constant 0 : i32
+  hlfir.assign %c0_i32_0 to %7#0 : i32, !fir.ref<!fir.array<2x2xi32>>
+  return
+}
+
+// CHECK-LABEL:   func.func private @_QFPtest7({{.*}}) {{.*}} {
+// CHECK:           %[[VAL_0:.*]] = arith.constant 0 : i32
+// CHECK:           fir.do_loop %[[VAL_1:.*]] = %{{.*}} to %{{.*}} step %{{.*}} unordered {
+// CHECK:             %[[VAL_2:.*]] = hlfir.designate %{{.*}} (%[[VAL_1]]) : (!fir.ref<!fir.array<4xi32>>, index) -> !fir.ref<i32>
+// CHECK:             hlfir.assign %[[VAL_0]] to %[[VAL_2]] : i32, !fir.ref<i32>
+// CHECK:           }
+// CHECK:         }

@luporl luporl requested review from vzakhari and eugeneepshteyn July 7, 2025 18:36
@luporl
Copy link
Contributor Author

luporl commented Jul 7, 2025

Fix for issue reported in #146408.

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.

Thank you!

@eugeneepshteyn
Copy link
Contributor

Thank you for the quick fix!

@@ -805,13 +805,12 @@ llvm::LogicalResult BroadcastAssignBufferization::matchAndRewrite(
shape, /*slice=*/mlir::Value{});
} else {
// Array references must have fixed shape, when used in assignments.
auto refTy = mlir::cast<fir::ReferenceType>(lhs.getType());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is slightly unsafe to assume this cannot be a fir.heap/fir.ptr. Nothing prevents these types from describing constant shaped compiler generated objects. Please use fir::unwrapRefType() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review.

On line 786 there is a call to hlfir::derefPointersAndAllocatables:
lhs = hlfir::derefPointersAndAllocatables(loc, builder, lhs);
Shouldn't it handle fir.heap/fir.ptr already?

Also, after getting the flat array reference type, lhs is converted to it. Would this convert be valid for fir.heap/fir.ptr?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to submit this PR soon and do a follow-up with any additional changes? This issue blocks internal testing of some apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it is already approved and blocking testing, I believe it's OK to merge it now.
As mentioned, I can make any necessary changes in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I understand @jeanPerier's comment.
Even after calling hlfir::derefPointersAndAllocatables the resulting type may still be a fir.heap/fir.ptr.
They can be converted to a fir.ref.

It seems hlfir::derefPointersAndAllocatables dereferences other pointer types, such as !fir.ref<!fir.box<!fir.ptr<!fir.array...>>>, generated from Fortran pointers, but not pointers directly to arrays, such as !fir.ptr<!fir.array...>, that may appear when equivalence is used.

I'll prepare a follow-up PR with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, lesson learned: "always listen to Jean" :)

@luporl luporl merged commit e976eaf into llvm:main Jul 8, 2025
12 checks passed
@luporl luporl deleted the luporl-fix-opt-array-ini branch July 8, 2025 17:47
@eugeneepshteyn
Copy link
Contributor

eugeneepshteyn commented Jul 9, 2025

@luporl , sorry to bother you, but it seems this fix caused regression in various Fujitsu tests at -O1 or higher. For example, compiling https://github.com/fujitsu/compiler-test-suite/blob/main/Fortran/1032/1032_0022.f90 results in the following:

$ flang -c 1032_0022.f90 -O1
flang: llvm/include/llvm/Support/Casting.h:566: decltype(auto) llvm::cast(const From&) [with To = fir::ReferenceType; From = mlir::Type]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
...
#10 0x0000000005a38f49 (anonymous namespace)::BroadcastAssignBufferization::matchAndRewrite(hlfir::AssignOp, mlir::PatternRewrite
r&) const OptimizedBufferization.cpp:0:0
#11 0x00000000059dc3f3 mlir::detail::OpOrInterfaceRewritePatternBase<hlfir::AssignOp>::matchAndRewrite(mlir::Operation*, mlir::Pa
tternRewriter&) const ConvertToFIR.cpp:0:0          
#12 0x0000000008d2629c mlir::PatternApplicator::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&, llvm::function_ref<bool
 (mlir::Pattern const&)>, llvm::function_ref<void (mlir::Pattern const&)>, llvm::function_ref<llvm::LogicalResult (mlir::Pattern 
const&)>) (/proj/nv/llvm/Linux_x86_64/llvm-5497/bin/flang+0x8d2629c)
#13 0x0000000008cf153c (anonymous namespace)::GreedyPatternRewriteDriver::processWorklist() GreedyPatternRewriteDriver.cpp:0:0
#14 0x0000000008cf445c mlir::applyPatternsGreedily(mlir::Region&, mlir::FrozenRewritePatternSet const&, mlir::GreedyRewriteConfig
, bool*) (/proj/nv/llvm/Linux_x86_64/llvm-5497/bin/flang+0x8cf445c)
#15 0x0000000005a36317 (anonymous namespace)::OptimizedBufferizationPass::runOnOperation() OptimizedBufferization.cpp:0:0
#16 0x000000000a43d431 mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned 
int) (/proj/nv/llvm/Linux_x86_64/llvm-5497/bin/flang+0xa43d431)
#17 0x000000000a43d83a mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager
, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) (/proj/nv/llvm/Linux_x86_64/
llvm-5497/bin/flang+0xa43d83a)
#18 0x000000000a43dbbe mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::'lambda'(mlir::detail::OpToOpPassAdaptor::
runOnOperationAsyncImpl(bool)::OpPMInfo&)::operator()(mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo&) 
const Pass.cpp:0:0
#19 0x000000000a43dd8f std::_Function_handler<void (), llvm::LogicalResult mlir::failableParallelForEach<__gnu_cxx::__normal_iter
ator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo*, std::vector<mlir::detail::OpToOpPassAdaptor::runO
nOperationAsyncImpl(bool)::OpPMInfo, std::allocator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo>>>, 
void mlir::parallelForEach<__gnu_cxx::__normal_iterator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo*
, std::vector<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo, std::allocator<mlir::detail::OpToOpPassAd
aptor::runOnOperationAsyncImpl(bool)::OpPMInfo>>>, mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::'lambda'(mlir:
:detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo&)>(mlir::MLIRContext*, __gnu_cxx::__normal_iterator<mlir::det
ail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo*, std::vector<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyn
cImpl(bool)::OpPMInfo, std::allocator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo>>>, __gnu_cxx::__n
ormal_iterator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo*, std::vector<mlir::detail::OpToOpPassAda
ptor::runOnOperationAsyncImpl(bool)::OpPMInfo, std::allocator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpP
MInfo>>>, mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::'lambda'(mlir::detail::OpToOpPassAdaptor::runOnOperatio
nAsyncImpl(bool)::OpPMInfo&)&&)::'lambda'(__gnu_cxx::__normal_iterator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(b
ool)::OpPMInfo*, std::vector<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo, std::allocator<mlir::detai
l::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo>>>&&)>(mlir::MLIRContext*, __gnu_cxx::__normal_iterator<mlir::detai
l::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo*, std::vector<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncI
mpl(bool)::OpPMInfo, std::allocator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo>>>, __gnu_cxx::__nor
mal_iterator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMInfo*, std::vector<mlir::detail::OpToOpPassAdapt
or::runOnOperationAsyncImpl(bool)::OpPMInfo, std::allocator<mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::OpPMI
nfo>>>, mlir::detail::OpToOpPassAdaptor::runOnOperationAsyncImpl(bool)::'lambda'(mlir::detail::OpToOpPassAdaptor::runOnOperationA
syncImpl(bool)::OpPMInfo&)&&)::'lambda'()>::_M_invoke(std::_Any_data const&) Pass.cpp:0:0
...

Known flang version with the crash:

$ flang --version
flang version 21.0.0git (https://github.com/llvm/llvm-project 46caad52ac14cefd6f9cf3188863818e330f3844)
Target: x86_64-unknown-linux-gnu

Known flang version without the crash:

$ flang --version
flang version 21.0.0git (https://github.com/llvm/llvm-project a7a7e95720226da2e78b87dbd9006bd4113c85ea)
Target: x86_64-unknown-linux-gnu

luporl added a commit to luporl/llvm-project that referenced this pull request Jul 9, 2025
In some cases fixed shape arrays can be fir.heap/fir.ptr, even
after hlfir::derefPointersAndAllocatables() is called.
luporl added a commit to luporl/llvm-project that referenced this pull request Jul 9, 2025
In some cases fixed shape arrays can be fir.heap/fir.ptr, even
after hlfir::derefPointersAndAllocatables() is called.
@luporl
Copy link
Contributor Author

luporl commented Jul 9, 2025

@eugeneepshteyn, sorry for introducing another regression.

#147761 should fix it.

luporl added a commit that referenced this pull request Jul 9, 2025
In some cases fixed shape arrays can be fir.heap/fir.ptr, even
after hlfir::derefPointersAndAllocatables() is called.
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.

5 participants