-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[flang] Fix length handling in character kind implicit conversion #74586
Conversation
@llvm/pr-subscribers-flang-fir-hlfir Author: None (jeanPerier) ChangesWhen assigning to a whole allocatable, lowering is dealing with the implicit conversion to preserve the RHS lower bounds. In case of character KIND mismatch, the code was setting the new RHS length to the one from the LHS, which is wrong for two reasons:
Update the code to preserve the RHS type length when materializing the implicit character KIND conversion. Full diff: https://github.com/llvm/llvm-project/pull/74586.diff 2 Files Affected:
diff --git a/flang/lib/Optimizer/Builder/HLFIRTools.cpp b/flang/lib/Optimizer/Builder/HLFIRTools.cpp
index 0b1e57e8f6c37..17efa45b8667d 100644
--- a/flang/lib/Optimizer/Builder/HLFIRTools.cpp
+++ b/flang/lib/Optimizer/Builder/HLFIRTools.cpp
@@ -1132,8 +1132,13 @@ hlfir::genTypeAndKindConvert(mlir::Location loc, fir::FirOpBuilder &builder,
std::optional<int> toKindCharConvert;
if (auto toCharTy = mlir::dyn_cast<fir::CharacterType>(toType)) {
if (auto fromCharTy = mlir::dyn_cast<fir::CharacterType>(fromType))
- if (toCharTy.getFKind() != fromCharTy.getFKind())
+ if (toCharTy.getFKind() != fromCharTy.getFKind()) {
toKindCharConvert = toCharTy.getFKind();
+ // Preserve source length (padding/truncation will occur in assignment
+ // if needed).
+ toType = fir::CharacterType::get(
+ fromType.getContext(), toCharTy.getFKind(), fromCharTy.getLen());
+ }
// Do not convert in case of character length mismatch only, hlfir.assign
// deals with it.
if (!toKindCharConvert)
diff --git a/flang/test/Lower/HLFIR/implicit-type-conversion-allocatable.f90 b/flang/test/Lower/HLFIR/implicit-type-conversion-allocatable.f90
index 7083a825dfd3b..361cd61adea2d 100644
--- a/flang/test/Lower/HLFIR/implicit-type-conversion-allocatable.f90
+++ b/flang/test/Lower/HLFIR/implicit-type-conversion-allocatable.f90
@@ -38,3 +38,29 @@ subroutine preserve_lbounds(x, y)
! CHECK: hlfir.destroy %[[VAL_8]] : !hlfir.expr<?xi32>
! CHECK: return
! CHECK: }
+
+! Test that RHS character length is preserved in a character KIND
+! conversion before the assignment.
+subroutine kind_and_length(a, b)
+ character(len=4,kind=4), allocatable :: a(:)
+ character(len=2,kind=1) :: b(:)
+ a = b
+end subroutine
+! CHECK-LABEL: func.func @_QPkind_and_length(
+! CHECK: %[[VAL_2:.*]] = arith.constant 4 : index
+! CHECK: %[[VAL_3:.*]]:2 = hlfir.declare %[[VAL_0:[a-z0-9]*]] typeparams %[[VAL_2:[a-z0-9]*]] {{.*}}Ea
+! CHECK: %[[VAL_4:.*]] = arith.constant 2 : index
+! CHECK: %[[VAL_5:.*]]:2 = hlfir.declare %[[VAL_1:[a-z0-9]*]] typeparams %[[VAL_4:[a-z0-9]*]] {{.*}}Eb
+! CHECK: %[[VAL_6:.*]] = arith.constant 0 : index
+! CHECK: %[[VAL_7:.*]]:3 = fir.box_dims %[[VAL_5]]#0, %[[VAL_6]] : (!fir.box<!fir.array<?x!fir.char<1,2>>>, index) -> (index, index, index)
+! CHECK: %[[VAL_8:.*]] = fir.shape %[[VAL_7]]#1 : (index) -> !fir.shape<1>
+! CHECK: %[[VAL_9:.*]] = hlfir.elemental %[[VAL_8]] typeparams %[[VAL_4]] unordered : (!fir.shape<1>, index) -> !hlfir.expr<?x!fir.char<4,2>> {
+! CHECK: ^bb0(%[[VAL_10:.*]]: index):
+! CHECK: %[[VAL_11:.*]] = hlfir.designate %[[VAL_5]]#0 (%[[VAL_10]]) typeparams %[[VAL_4]] : (!fir.box<!fir.array<?x!fir.char<1,2>>>, index, index) -> !fir.ref<!fir.char<1,2>>
+! CHECK: %[[VAL_12:.*]] = fir.alloca !fir.char<4,?>(%[[VAL_4]] : index)
+! CHECK: fir.char_convert %[[VAL_11]] for %[[VAL_4]] to %[[VAL_12]] : !fir.ref<!fir.char<1,2>>, index, !fir.ref<!fir.char<4,?>>
+! CHECK: %[[VAL_13:.*]]:2 = hlfir.declare %[[VAL_12:[a-z0-9]*]] typeparams %[[VAL_4:[a-z0-9]*]] {uniq_name = ".temp.kindconvert"} : (!fir.ref<!fir.char<4,?>>, index) -> (!fir.boxchar<4>, !fir.ref<!fir.char<4,?>>)
+! CHECK: hlfir.yield_element %[[VAL_13]]#0 : !fir.boxchar<4>
+! CHECK: }
+! CHECK: hlfir.assign %[[VAL_9]] to %[[VAL_3]]#0 realloc keep_lhs_len : !hlfir.expr<?x!fir.char<4,2>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?x!fir.char<4,4>>>>>
+
|
When assigning to a whole allocatable, lowering is dealing with the implicit conversion to preserve the RHS lower bounds. In case of character KIND mismatch, the code was setting the new RHS length to the one from the LHS, which is wrong for two reasons: - no padding/truncation was actually done in the conversion - the RHS length should not be touched since the one from the allocatable LHS may change to become the one of the RHS. Update the code to preserve the RHS type length.
4069902
to
65b9e1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All builds and tests correctly and looks good.
I wonder about the update message, which discusses setting the new RHS length. Should this say the new LHS length?
Thanks for the review! The "new RHS" is a new value converted from the RHS of the original fortran code. The updated code is producing this "new RHS" and was incorrectly setting its MLIR type length to the one of the assignment LHS (instead of preserving the one from the original RHS). |
When assigning to a whole allocatable, lowering is dealing with the implicit conversion to preserve the RHS lower bounds. In case of character KIND mismatch, the code was setting the new RHS length to the one from the LHS, which is wrong for two reasons:
Update the code to preserve the RHS type length when materializing the implicit character KIND conversion.