Skip to content

[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

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

jeanPerier
Copy link
Contributor

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.

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Jan 30, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 30, 2025

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

Author: None (jeanPerier)

Changes

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.


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

2 Files Affected:

  • (modified) flang/include/flang/Optimizer/Builder/HLFIRTools.h (+1-1)
  • (modified) flang/test/HLFIR/assign-codegen.fir (+10)
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>

Copy link
Contributor

@razvanlupusoru razvanlupusoru 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!

@jeanPerier jeanPerier merged commit 242aa8c into llvm:main Jan 30, 2025
11 checks passed
@jeanPerier jeanPerier deleted the assign-scalar-box branch January 30, 2025 16:03
@tblah
Copy link
Contributor

tblah commented Jan 31, 2025

I getting two gfortran tests segfaulting at runtime. The bisect pointed me to this patch:

Reproduce with flang -o /tmp/out optional_absent_4.f90 && /tmp/out

Please could you take a look?

@DavidSpickett
Copy link
Collaborator

Just got the same result myself. This is failing on our bots but because they had existing failures, no notification went out.

https://lab.llvm.org/buildbot/#/builders/198/builds/1633

@DavidSpickett
Copy link
Collaborator

Smaller version:

module z
  implicit none
contains
  subroutine sum_2 (input, res, mask)
    logical, intent(in), optional :: mask
    integer, intent(in) :: input(:,:)
    integer, dimension(:), intent(out) :: res
    res = sum (input, dim=1, mask=mask)
  end subroutine sum_2
end module z

program main
  use z
  implicit none
  integer :: i2(2,3) = reshape([1,2,4,8,16,32], [2,3])
  integer, dimension(3) :: res3
  res3 = -2
  call sum_2 (i2, res3)
  if (any (res3 /= [3, 12, 48])) stop 2
end program main

@jeanPerier
Copy link
Contributor Author

I getting two gfortran tests segfaulting at runtime. The bisect pointed me to this patch:

Thanks Tom and David, I saw that too this morning, I have a fix here: #125215

DavidSpickett pushed a commit that referenced this pull request Jan 31, 2025
…ented value" (#125222)

Reverts #125059

Broke OPTIONAL lowering for some intrinsics.

I have a proper fix for review
#125215, but I would like to
better test it, so I am reverting in the meantime.
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 31, 2025
…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.
jeanPerier added a commit to jeanPerier/llvm-project that referenced this pull request Jan 31, 2025
…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.
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