-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[FLANG] Solving issue with adjustr intrinsic in where construct #146851
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Ebin-McW (EbinJose2002) ChangesSolves #122823 Full diff: https://github.com/llvm/llvm-project/pull/146851.diff 2 Files Affected:
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
index 58f2b57712974..240b79138b0e1 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/BufferizeHLFIR.cpp
@@ -446,7 +446,8 @@ struct AssociateOpConversion
((mlir::isa<fir::BoxCharType>(sourceVar.getType()) &&
!mlir::isa<fir::BoxCharType>(assocType)))) {
sourceVar = builder.create<fir::BoxAddrOp>(loc, assocType, sourceVar);
- } else {
+ } else if (!mlir::isa<fir::ReferenceType>(sourceVar.getType()) ||
+ !mlir::isa<fir::BoxCharType>(assocType)) {
sourceVar = builder.createConvert(loc, assocType, sourceVar);
}
return sourceVar;
diff --git a/flang/test/Lower/adjustr.f90 b/flang/test/Lower/adjustr.f90
new file mode 100644
index 0000000000000..4813834614bd6
--- /dev/null
+++ b/flang/test/Lower/adjustr.f90
@@ -0,0 +1,14 @@
+! RUN: bbc -emit-fir -o - %s | FileCheck %s
+
+! Checking no reference to boxchar conversion is in the emitted fir.
+! CHECK-NOT: (!fir.ref<!fir.char<1,4>>) -> !fir.boxchar<1>
+
+program main
+ logical ,dimension(1):: mask = .true.
+ character(len=2),dimension(1):: d1 = "a "
+ character(len=4),dimension(1):: d4
+ where (mask)
+ d4 = adjustr(d1 // d1)
+ end where
+ write(6,*) "d4 =", d4
+end
|
|
Thank you for the changes, but I think we cannot just avoid generating proper "conversion" here. Maybe it is not the case currently, but imagine that both results of |
vzakhari
left a comment
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.
Please generate fir.emboxchar instead.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
Thanks for the update!
This will only work for the types with constant length. I do not know if we should be here for non-constant lengths, but I would just take the type parameters from the associate operation - this should work for all the cases.
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.
Sorry for the delayed reply.
I have made the change to access the length from associate instruction itself.
flang/test/Lower/adjustr.f90
Outdated
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.
Please update the checks to show what kind of emboxchar we generate after this change.
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.
I have added check for the emboxchar that is generated.
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.
Please add more context into the checks, e.g. the operands of fir.emboxchar, where they are coming from, and how the results of fir.emboxchar are used.
| sourceVar = builder.create<fir::BoxAddrOp>(loc, assocType, sourceVar); | ||
| } else if (mlir::isa<fir::ReferenceType>(sourceVar.getType()) && | ||
| mlir::isa<fir::BoxCharType>(assocType)) { | ||
| mlir::Value lenVal = associate.getTypeparams()[0]; |
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.
nit: please assert that getTypeparams() returns a vector of length one.
flang/test/Lower/adjustr.f90
Outdated
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.
Please add more context into the checks, e.g. the operands of fir.emboxchar, where they are coming from, and how the results of fir.emboxchar are used.
Co-authored-by: Slava Zakharin <[email protected]>
Fixes #122823
An unnecessary convert instruction was generated in the case of reference to boxchar conversion. So excluded them in the condition. The output is generating correctly.