[flang] Fix optimization of array assignments after #146408 #147371
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Leandro Lupori (luporl) ChangesHost associated variables were not being handled properly. Full diff: https://github.com/llvm/llvm-project/pull/147371.diff 2 Files Affected:
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: }
|
|
Fix for issue reported in #146408. |
|
Thank you for the quick fix! |
jeanPerier
approved these changes
Jul 8, 2025
| @@ -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()); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, lesson learned: "always listen to Jean" :)
|
@luporl , sorry to bother you, but it seems this fix caused regression in various Fujitsu tests at Known flang version with the crash: Known flang version without the crash: |
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.
|
@eugeneepshteyn, sorry for introducing another regression. #147761 should fix it. |
luporl
added a commit
that referenced
this pull request
Jul 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.