Skip to content

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