[Flang] Do not provide a shape operand for box addresses

[ceseo]

Box addresses already contain shape information. Do not provide an additional shape operand.

Fixes #132648

[llvmbot]

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

Author: Carlos Seo (ceseo)

Changes

Box addresses already contain shape information. Do not provide an additional shape operand.

Fixes #132648

---

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

2 Files Affected:

• (modified) flang/lib/Lower/ConvertVariable.cpp (+6-2)
• (added) flang/test/Lower/box-address.f90 (+34)

diff --git a/flang/lib/Lower/ConvertVariable.cpp b/flang/lib/Lower/ConvertVariable.cpp
index c79c9b1ab0f51..4b6523705a42b 100644
--- a/flang/lib/Lower/ConvertVariable.cpp
+++ b/flang/lib/Lower/ConvertVariable.cpp
@@ -2008,9 +2008,13 @@ static void genDeclareSymbol(Fortran::lower::AbstractConverter &converter,
     if (converter.isRegisteredDummySymbol(sym))
       dummyScope = converter.dummyArgsScopeValue();
     auto [storage, storageOffset] = converter.getSymbolStorage(sym);
+    // For box addresses, shape information is already included, so we must
+    // not provide a shape operand.
+    mlir::Value shapeForDeclare =
+        fir::isBoxAddress(base.getType()) ? mlir::Value{} : shapeOrShift;
     auto newBase = hlfir::DeclareOp::create(
-        builder, loc, base, name, shapeOrShift, lenParams, dummyScope, storage,
-        storageOffset, attributes, dataAttr);
+        builder, loc, base, name, shapeForDeclare, lenParams, dummyScope,
+        storage, storageOffset, attributes, dataAttr);
     symMap.addVariableDefinition(sym, newBase, force);
     return;
   }
diff --git a/flang/test/Lower/box-address.f90 b/flang/test/Lower/box-address.f90
new file mode 100644
index 0000000000000..04f14188a7bec
--- /dev/null
+++ b/flang/test/Lower/box-address.f90
@@ -0,0 +1,34 @@
+! RUN: flang -fc1 -emit-hlfir %s -o - | FileCheck %s
+
+module m3
+  type x1
+     integer::ix1
+  end type x1
+  type,extends(x1)::x2
+  end type x2
+  type,extends(x2)::x3
+  end type x3
+  class(x1),pointer,dimension(:)::cy1
+contains
+  subroutine dummy()
+  entry      chk(c1)
+   class(x1),dimension(3)::c1
+ end subroutine dummy
+end module m3
+! CHECK-LABEL: func.func @_QMm3Pchk(
+! CHECK-SAME:    %[[ARG0:.*]]: !fir.class<!fir.array<3x!fir.type<_QMm3Tx1{ix1:i32}>>> {fir.bindc_name = "c1"}) {
+! CHECK:         %[[DUMMY_SCOPE:.*]] = fir.dummy_scope : !fir.dscope
+! CHECK:         %[[DECLARE:.*]]:2 = hlfir.declare %[[ARG0]] dummy_scope %[[DUMMY_SCOPE]] {uniq_name = "_QMm3FdummyEc1"} : (!fir.class<!fir.array<3x!fir.type<_QMm3Tx1{ix1:i32}>>>, !fir.dscope) -> (!fir.class<!fir.array<3x!fir.type<_QMm3Tx1{ix1:i32}>>>, !fir.class<!fir.array<3x!fir.type<_QMm3Tx1{ix1:i32}>>>)
+
+subroutine s1
+  use m3
+  type(x1),target::ty1(3)
+  ty1%ix1=[1,2,3]
+  cy1=>ty1
+  call chk(cy1)
+end subroutine s1
+
+program main
+  call s1
+  print *,'pass'
+end program main

[jeanPerier]

Thanks for working on a fix!

I need to double check something with a more complex scenario here to verify this fix is OK.

Basically the root cause is that genUnusedEntryPointBox() is not called here in Fortran::lower::mapSymbolAttributes because of the static shape aspect and the code is falling down in a weird path a creating a fir.ref<fir.class> for a symbol that is expected to be a fir.class and genDeclareSymbol is then created with something weird/unexpected.

While your fix allows your example to compile, I am afraid that with cases where there are "protected uses" of the unused entry argument that would remain, not having an hlfir.declare with a fir.class argument would cause other troubles.

[ceseo]

While your fix allows your example to compile, I am afraid that with cases where there are "protected uses" of the unused entry argument that would remain, not having an hlfir.declare with a fir.class argument would cause other troubles.

Please let me know and I'll make necessary adjustments.

[ceseo]

@jeanPerier any other thoughts on this?

[jeanPerier]

@jeanPerier any other thoughts on this?

Yes, I think it is best to properly create the box in Fortran::lower::mapSymbolAttributes.

I would suggest adding the following right below this line:

if (isUnusedEntryDummy && llvm::isa<fir::BaseBoxType>(converter.genType(var)))
  if (genUnusedEntryPointBox())
    return;

Deriving 1e1f60c example, I could not quickly come with a problematic example that would break/crash with your solution, but I am a bit concerned some of the lowering code may not be happy to see a fir.ref<fir.class> for a Symbol that is not a POINTER/ALLOCATABLE since this is never the case.

[ceseo]

Yes, I think it is best to properly create the box in Fortran::lower::mapSymbolAttributes.

OK, got it! Thanks!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jeanPerier]

Thanks!