-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[flang][hlfir] get rid of box when translating scalars to extented value #125059
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
Conversation
@llvm/pr-subscribers-flang-fir-hlfir Author: None (jeanPerier) ChangesThe code in This created issues downstream because utilities using fir::ExtendedValue were not implemented to work with intrinsic scalars fir.box. fir.box of intrinsic scalars are not very commonly used as hlfir::Entity but they are allowed and should work where accepted. Full diff: https://github.com/llvm/llvm-project/pull/125059.diff 2 Files Affected:
diff --git a/flang/include/flang/Optimizer/Builder/HLFIRTools.h b/flang/include/flang/Optimizer/Builder/HLFIRTools.h
index 0684ad0f926ec9..d8785969bb7247 100644
--- a/flang/include/flang/Optimizer/Builder/HLFIRTools.h
+++ b/flang/include/flang/Optimizer/Builder/HLFIRTools.h
@@ -125,7 +125,7 @@ class Entity : public mlir::Value {
bool isSimplyContiguous() const {
// If this can be described without a fir.box in FIR, this must
// be contiguous.
- if (!hlfir::isBoxAddressOrValueType(getFirBase().getType()))
+ if (!hlfir::isBoxAddressOrValueType(getFirBase().getType()) || isScalar())
return true;
// Otherwise, if this entity has a visible declaration in FIR,
// or is the dereference of an allocatable or contiguous pointer
diff --git a/flang/test/HLFIR/assign-codegen.fir b/flang/test/HLFIR/assign-codegen.fir
index 581d1ab0e7739c..5e48784284a8b4 100644
--- a/flang/test/HLFIR/assign-codegen.fir
+++ b/flang/test/HLFIR/assign-codegen.fir
@@ -427,3 +427,13 @@ func.func @test_upoly_expr_assignment(%arg0: !fir.class<!fir.array<?xnone>> {fir
// CHECK: }
// CHECK: return
// CHECK: }
+
+func.func @test_scalar_box(%arg0: f32, %arg1: !fir.box<!fir.ptr<f32>>) {
+ hlfir.assign %arg0 to %arg1 : f32, !fir.box<!fir.ptr<f32>>
+ return
+}
+// CHECK-LABEL: func.func @test_scalar_box(
+// CHECK-SAME: %[[VAL_0:.*]]: f32,
+// CHECK-SAME: %[[VAL_1:.*]]: !fir.box<!fir.ptr<f32>>) {
+// CHECK: %[[VAL_2:.*]] = fir.box_addr %[[VAL_1]] : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
+// CHECK: fir.store %[[VAL_0]] to %[[VAL_2]] : !fir.ptr<f32>
|
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.
Thank you!
I getting two gfortran tests segfaulting at runtime. The bisect pointed me to this patch:
Reproduce with Please could you take a look? |
Just got the same result myself. This is failing on our bots but because they had existing failures, no notification went out. |
Smaller version:
|
Thanks Tom and David, I saw that too this morning, I have a fix here: #125215 |
…lars to extented value" (#125222) Reverts llvm/llvm-project#125059 Broke OPTIONAL lowering for some intrinsics. I have a proper fix for review llvm/llvm-project#125215, but I would like to better test it, so I am reverting in the meantime.
…lue (llvm#125059) The code in `translateToExtendedValue(hlfir::Entity)` was not getting rid of the fir.box for scalars because isSimplyContiguous() returned false for them. This created issues downstream because utilities using fir::ExtendedValue were not implemented to work with intrinsic scalars fir.box. fir.box of intrinsic scalars are not very commonly used as hlfir::Entity but they are allowed and should work where accepted.
The code in
translateToExtendedValue(hlfir::Entity)
was not getting rid of the fir.box for scalars because isSimplyContiguous() returned false for them.This created issues downstream because utilities using fir::ExtendedValue were not implemented to work with intrinsic scalars fir.box.
fir.box of intrinsic scalars are not very commonly used as hlfir::Entity but they are allowed and should work where accepted.