[flang][debug] Support assumed size arrays. #96316
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Here we don't know the size of last dimension and use a null subrange for that dimension. User will have to provide the dimension when evaluating variable of this type. The debugging looks like as follows: subroutine fn(a1, a2) integer a1(5, *), a2(*) ... end (gdb) p a1(1,2) $2 = 2 (gdb) p a2(3) $3 = 3 (gdb) ptype a1 type = integer (5,*) (gdb) ptype a2 type = integer (*)
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Abid Qadeer (abidh) ChangesHere we don't know the size of last dimension and use a null subrange for that. User will have to provide the dimension when evaluating variable of this type. The debugging looks like as follows: subroutine fn(a1, a2) (gdb) p a1(1,2) Full diff: https://github.com/llvm/llvm-project/pull/96316.diff 3 Files Affected:
diff --git a/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp b/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
index 407ecc8e327b4..5bf938aeaee5e 100644
--- a/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
+++ b/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
@@ -155,28 +155,31 @@ mlir::LLVM::DITypeAttr DebugTypeGenerator::convertSequenceType(
fir::SequenceType seqTy, mlir::LLVM::DIFileAttr fileAttr,
mlir::LLVM::DIScopeAttr scope, mlir::Location loc) {
mlir::MLIRContext *context = module.getContext();
- // FIXME: Only fixed sizes arrays handled at the moment.
- if (seqTy.hasDynamicExtents())
- return genPlaceholderType(context);
llvm::SmallVector<mlir::LLVM::DINodeAttr> elements;
mlir::LLVM::DITypeAttr elemTy =
convertType(seqTy.getEleTy(), fileAttr, scope, loc);
for (fir::SequenceType::Extent dim : seqTy.getShape()) {
- auto intTy = mlir::IntegerType::get(context, 64);
- // FIXME: Only supporting lower bound of 1 at the moment. The
- // 'SequenceType' has information about the shape but not the shift. In
- // cases where the conversion originated during the processing of
- // 'DeclareOp', it may be possible to pass on this information. But the
- // type conversion should ideally be based on what information present in
- // the type class so that it works from everywhere (e.g. when it is part
- // of a module or a derived type.)
- auto countAttr = mlir::IntegerAttr::get(intTy, llvm::APInt(64, dim));
- auto lowerAttr = mlir::IntegerAttr::get(intTy, llvm::APInt(64, 1));
- auto subrangeTy = mlir::LLVM::DISubrangeAttr::get(
- context, countAttr, lowerAttr, nullptr, nullptr);
- elements.push_back(subrangeTy);
+ if (dim == seqTy.getUnknownExtent()) {
+ auto subrangeTy = mlir::LLVM::DISubrangeAttr::get(
+ context, nullptr, nullptr, nullptr, nullptr);
+ elements.push_back(subrangeTy);
+ } else {
+ auto intTy = mlir::IntegerType::get(context, 64);
+ // FIXME: Only supporting lower bound of 1 at the moment. The
+ // 'SequenceType' has information about the shape but not the shift. In
+ // cases where the conversion originated during the processing of
+ // 'DeclareOp', it may be possible to pass on this information. But the
+ // type conversion should ideally be based on what information present in
+ // the type class so that it works from everywhere (e.g. when it is part
+ // of a module or a derived type.)
+ auto countAttr = mlir::IntegerAttr::get(intTy, llvm::APInt(64, dim));
+ auto lowerAttr = mlir::IntegerAttr::get(intTy, llvm::APInt(64, 1));
+ auto subrangeTy = mlir::LLVM::DISubrangeAttr::get(
+ context, countAttr, lowerAttr, nullptr, nullptr);
+ elements.push_back(subrangeTy);
+ }
}
// Apart from arrays, the `DICompositeTypeAttr` is used for other things like
// structure types. Many of its fields which are not applicable to arrays
diff --git a/flang/test/Integration/debug-assumed-size-array.f90 b/flang/test/Integration/debug-assumed-size-array.f90
new file mode 100644
index 0000000000000..efa53e643ecbb
--- /dev/null
+++ b/flang/test/Integration/debug-assumed-size-array.f90
@@ -0,0 +1,20 @@
+! RUN: %flang_fc1 -emit-llvm -debug-info-kind=standalone %s -o - | FileCheck %s
+
+module helper
+ implicit none
+ contains
+ subroutine fn (a1, a2)
+ integer a1(5, *), a2(*)
+ print *, a1(1,1)
+ print *, a2(2)
+ end subroutine fn
+end module helper
+
+! CHECK-DAG: ![[TY1:[0-9]+]] = !DICompositeType(tag: DW_TAG_array_type{{.*}}elements: ![[ELEMS1:[0-9]+]]{{.*}})
+! CHECK-DAG: ![[ELEMS1]] = !{![[ELM1:[0-9]+]], ![[EMPTY:[0-9]+]]}
+! CHECK-DAG: ![[ELM1]] = !DISubrange(count: 5, lowerBound: 1)
+! CHECK-DAG: ![[EMPTY]] = !DISubrange()
+! CHECK-DAG: ![[TY2:[0-9]+]] = !DICompositeType(tag: DW_TAG_array_type{{.*}}elements: ![[ELEMS2:[0-9]+]]{{.*}})
+! CHECK-DAG: ![[ELEMS2]] = !{![[EMPTY:[0-9]+]]}
+! CHECK-DAG: !DILocalVariable(name: "a1"{{.*}}type: ![[TY1:[0-9]+]])
+! CHECK-DAG: !DILocalVariable(name: "a2"{{.*}}type: ![[TY2:[0-9]+]])
diff --git a/flang/test/Transforms/debug-assumed-size-array.fir b/flang/test/Transforms/debug-assumed-size-array.fir
new file mode 100644
index 0000000000000..d25224fb1b5ec
--- /dev/null
+++ b/flang/test/Transforms/debug-assumed-size-array.fir
@@ -0,0 +1,21 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+ func.func @_QMhelperPfn(%arg0: !fir.ref<!fir.array<5x?xi32>> {fir.bindc_name = "a1"}, %arg1: !fir.ref<!fir.array<?xi32>> {fir.bindc_name = "a2"}, %arg2: !fir.ref<!fir.array<2x?xi32>> {fir.bindc_name = "a3"}) {
+ %c5 = arith.constant 5 : index
+ %c1 = arith.constant 1 : index
+ %c-1 = arith.constant -1 : index
+ %0 = fir.undefined !fir.dscope
+ %1 = fircg.ext_declare %arg0(%c5, %c-1) dummy_scope %0 {uniq_name = "_QMhelperFfnEa1"} : (!fir.ref<!fir.array<5x?xi32>>, index, index, !fir.dscope) -> !fir.ref<!fir.array<5x?xi32>> loc(#loc1)
+ %2 = fircg.ext_declare %arg1(%c-1) dummy_scope %0 {uniq_name = "_QMhelperFfnEa2"} : (!fir.ref<!fir.array<?xi32>>, index, !fir.dscope) -> !fir.ref<!fir.array<?xi32>> loc(#loc2)
+ return
+ } loc(#loc3)
+}
+#loc3 = loc("test.f90":1:1)
+#loc1 = loc("test.f90":3:1)
+#loc2 = loc("test.f90":4:1)
+
+// CHECK-DAG: #[[TY1:.*]] = #llvm.di_composite_type<tag = DW_TAG_array_type{{.*}}elements = #llvm.di_subrange<count = 5 : i64, lowerBound = 1 : i64>, #llvm.di_subrange<>>
+// CHECK-DAG: #[[TY2:.*]] = #llvm.di_composite_type<tag = DW_TAG_array_type{{.*}}elements = #llvm.di_subrange<>>
+// CHECK-DAG: #llvm.di_local_variable<{{.*}}name = "a1"{{.*}}type = #[[TY1]]>
+// CHECK-DAG: #llvm.di_local_variable<{{.*}}name = "a2"{{.*}}type = #[[TY2]]>
|
abidh
added a commit
to abidh/llvm-project
that referenced
this pull request
Jun 21, 2024
This PR is required for llvm#96316. For assumed size arrays, we don't have the size of the array in the last dimension. This is represented in metadata by having a DISubrange with no entry for count or upper bound. LLVM verifier allows a DISubrange with no count or upper bound only for Fortran. This language is set when DICompileUnit is processed. If a type is processed before DICompileUnit then Verifier will not allow DISubrange with not count or upper bound entry. It will fail with error "Subrange must contain count or upperBound". Above scenario easily happens if a subroutine at global scope takes assumed size parameter. The metadata on DISubprogram is such that type is processed before unit so we hit Verifier::visitDISubrange before language has been set to Fortran. I have worked around this issue by setting the scope of such functions to be DICompileUnit instead of DIFile. As scope metadata in DISubprogram is processed before type so this problem does not happen. Some other observations about this change: 1. It does not make any difference to the generated DWARF. 2. It bring subroutine inline with module as modules already set their scope to DiCompileUnit. 3. I also looked at the classic flang compiler and it also uses DICompileUnit as scope for such subroutines probably for the same reasons.
Added comments on constant value parameters.
abidh
requested review from
jeanPerier,
dpalermo,
jinisusan,
kiranktp and
vzakhari
jeanPerier
reviewed
Jun 24, 2024
Comment on lines
164
to
168
| if (dim == seqTy.getUnknownExtent()) { | ||
| auto subrangeTy = mlir::LLVM::DISubrangeAttr::get( | ||
| context, /*count=*/nullptr, /*lowerBound=*/nullptr, | ||
| /*upperBound*/ nullptr, /*stride*/ nullptr); | ||
| elements.push_back(subrangeTy); |
There was a problem hiding this comment.
Will it be needed to make a difference between assumed-size and explicit shape with non constant extent here?
subroutine sub1(x, n)
real :: x(n)
end subroutine
and:
subroutine sub1(x)
real :: x(*)
end subroutine
There was a problem hiding this comment.
Thanks for catching this. We need to make a different between these 2 cases. For the 1st case, we can generate a DISubrange pointing to a variable. But that will require DeclOp to be propagated to DebugTypeConvertor. This information is not available in Type only.
For this PR, I have added a FIXME and a testcase with variable extent arrays that is currently xfailed. I will also open an issue to keep track of this once this PR is merged.
There was a problem hiding this comment.
Won't reading from the descriptor at runtime work just as well in this case? Does the debug output look different for reading from the variable?
There was a problem hiding this comment.
As I see there is no descriptor in this case. The type of a variable I get is !fir.array<?xf32>
There was a problem hiding this comment.
Ok, we should probably add some attribute to tag assumed-size array xxx.declare (we can currently infer that looking at the last extent value that should be a -1 constant, but chain lookups are a bit weak IMHO, better to set an attribute.
As I see there is no descriptor in this case.
To be more specific @tblah, there was one in HLFIR, but it is "unlink" when creating the fir.declare that has a single result. , and it may have been eliminated if unused in FIR.
Currently, the arrays with non constant extent are being handled like assumed size arrays. Added a FIXME to mention that there is a better way to handle them. Also added a testcase that is currently xfailed.
jeanPerier
approved these changes
Jun 25, 2024
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
Here we don't know the size of last dimension and use a null subrange for that. User will have to provide the dimension when evaluating variable of this type. The debugging looks like as follows: subroutine fn(a1, a2) integer a1(5, *), a2(*) ... end (gdb) p a1(1,2) $2 = 2 (gdb) p a2(3) $3 = 3 (gdb) ptype a1 type = integer (5,*) (gdb) ptype a2 type = integer (*)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here we don't know the size of last dimension and use a null subrange for that. User will have to provide the dimension when evaluating variable of this type. The debugging looks like as follows:
subroutine fn(a1, a2)
integer a1(5, ), a2()
...
end
(gdb) p a1(1,2)
$2 = 2
(gdb) p a2(3)
$3 = 3
(gdb) ptype a1
type = integer (5,)
(gdb) ptype a2
type = integer ()