[flang-commits] [flang] [flang][debug] Improve handling of dummy character arguments. (PR #108283)

[Abid Qadeer via flang-commits]

https://github.com/abidh created https://github.com/llvm/llvm-project/pull/108283

As described in #107998, we were not handling the case well when length of the character is not part of the type. This PR handles one of the case when the length can be calculated by looking at the result of corresponding `fir.unboxchar`.

The DIStringTypeAttr have a `stringLength` field that can be a variable. We create an artificial variable that will hold the length and used as value of `stringLength` field.

As we don't have any `fircg.ext_declare` to which we can attach the location. This PR attaches it to `fir.unboxchar` and we check for it during codegen and generate a `DbgValueOp`. I thought about the idea of generating a `fircg.ext_declare` so that we can attach the variable to it. That will avoid touching `fir.unboxchar` in codegen. But it would also have required adding an `alloca` to hold the value so that we can use `DbgDeclareOp`. I opted for `fir.unboxchar` as it seemed simpler.

Fixes #107998.

>From 15beec33271c5f08cdf8cd76d3739b98a298616e Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Tue, 3 Sep 2024 10:21:49 +0100
Subject: [PATCH] [flang][debug] Improve handling of dummy character arguments.

As described in #107998, we were not handling the case well when
length of the character is not part of the type. This PR handles one of
the case when the length can be calculated by looking at the result of
corresponding `fir.unboxchar`.

The DIStringTypeAttr have a `stringLength` field that can be a variable.
We create an artificial variable that will hold the length and used
as value of `stringLength` field.

As we don't have any `fircg.ext_declare` to which we can attach the
location. This PR attaches it to fir.unboxchar and we check for it
during CodeGen and generate a `DbgValueOp`. I thought about the idea of
generating a fircg.ext_declare so that we can attach the variable to it.
That will avoid touching `fir.unboxchar` in codegen. But it would also
have required adding an `alloca` to hold the value so that we can use
`DbgDeclareOp`. I opted for `fir.unboxchar`` as it seemed simpler.
---
 flang/lib/Optimizer/CodeGen/CodeGen.cpp       |  9 ++++++
 .../Transforms/DebugTypeGenerator.cpp         | 23 +++++++++++++--
 flang/test/Transforms/debug-107988.fir        | 29 +++++++++++++++++++
 3 files changed, 59 insertions(+), 2 deletions(-)
 create mode 100644 flang/test/Transforms/debug-107988.fir

diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index 88293bcf36a780..c65e7d40aa7c1e 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -3252,6 +3252,15 @@ struct UnboxCharOpConversion : public fir::FIROpConversion<fir::UnboxCharOp> {
     auto len = rewriter.create<mlir::LLVM::ExtractValueOp>(loc, tuple, 1);
     mlir::Value lenAfterCast = integerCast(loc, rewriter, lenTy, len);
 
+    // If a variable is attached to fir.unboxchar then generate a DbgValueOp
+    if (auto fusedLoc = mlir::dyn_cast<mlir::FusedLoc>(unboxchar.getLoc())) {
+      if (auto varAttr =
+              mlir::dyn_cast_or_null<mlir::LLVM::DILocalVariableAttr>(
+                  fusedLoc.getMetadata())) {
+        rewriter.create<mlir::LLVM::DbgValueOp>(lenAfterCast.getLoc(),
+                                                lenAfterCast, varAttr, nullptr);
+      }
+    }
     rewriter.replaceOp(unboxchar,
                        llvm::ArrayRef<mlir::Value>{ptrToBuffer, lenAfterCast});
     return mlir::success();
diff --git a/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp b/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
index 7c4382079fd6d4..eba2aad1b0851e 100644
--- a/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
+++ b/flang/lib/Optimizer/Transforms/DebugTypeGenerator.cpp
@@ -268,6 +268,7 @@ mlir::LLVM::DITypeAttr DebugTypeGenerator::convertCharacterType(
   uint64_t sizeInBits = 0;
   mlir::LLVM::DIExpressionAttr lenExpr = nullptr;
   mlir::LLVM::DIExpressionAttr locExpr = nullptr;
+  mlir::LLVM::DIVariableAttr varAttr = nullptr;
 
   if (hasDescriptor) {
     llvm::SmallVector<mlir::LLVM::DIExpressionElemAttr> ops;
@@ -286,7 +287,25 @@ mlir::LLVM::DITypeAttr DebugTypeGenerator::convertCharacterType(
     sizeInBits =
         charTy.getLen() * kindMapping.getCharacterBitsize(charTy.getFKind());
   } else {
-    return genPlaceholderType(context);
+    // In some situations, the len of the character is not part of the type but
+    // can be found at the runtime. Here we create an artificial variable that
+    // will contain that length. This variable is used as 'stringLength' in
+    // DIStringTypeAttr.
+    if (declOp && !declOp.getTypeparams().empty()) {
+      mlir::Operation *op = declOp.getTypeparams()[0].getDefiningOp();
+      if (auto unbox = mlir::dyn_cast_or_null<fir::UnboxCharOp>(op)) {
+        auto name =
+            mlir::StringAttr::get(context, "." + declOp.getUniqName().str());
+        mlir::LLVM::DITypeAttr Ty =
+            convertType(unbox.getResult(1).getType(), fileAttr, scope, declOp);
+        auto lvAttr = mlir::LLVM::DILocalVariableAttr::get(
+            context, scope, name, fileAttr, /*line=*/0, /*argNo=*/0,
+            /*alignInBits=*/0, Ty, mlir::LLVM::DIFlags::Artificial);
+        mlir::OpBuilder builder(context);
+        unbox->setLoc(builder.getFusedLoc({unbox->getLoc()}, lvAttr));
+        varAttr = mlir::cast<mlir::LLVM::DIVariableAttr>(lvAttr);
+      }
+    }
   }
 
   // FIXME: Currently the DIStringType in llvm does not have the option to set
@@ -296,7 +315,7 @@ mlir::LLVM::DITypeAttr DebugTypeGenerator::convertCharacterType(
   return mlir::LLVM::DIStringTypeAttr::get(
       context, llvm::dwarf::DW_TAG_string_type,
       mlir::StringAttr::get(context, ""), sizeInBits, /*alignInBits=*/0,
-      /*stringLength=*/nullptr, lenExpr, locExpr, encoding);
+      /*stringLength=*/varAttr, lenExpr, locExpr, encoding);
 }
 
 mlir::LLVM::DITypeAttr DebugTypeGenerator::convertPointerLikeType(
diff --git a/flang/test/Transforms/debug-107988.fir b/flang/test/Transforms/debug-107988.fir
new file mode 100644
index 00000000000000..0b9a1e7a4be2ee
--- /dev/null
+++ b/flang/test/Transforms/debug-107988.fir
@@ -0,0 +1,29 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s -o - | FileCheck %s
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s -o - | \
+// RUN: fir-opt --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" \
+// RUN: --mlir-print-debuginfo  | FileCheck --check-prefix=CODEGEN %s
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+  func.func @test(%arg0: !fir.ref<!fir.char<1,?>> {fir.bindc_name = "str"}, %arg1: i64) {
+    %0 = fir.emboxchar %arg0, %arg1 : (!fir.ref<!fir.char<1,?>>, i64) -> !fir.boxchar<1>
+    %1 = fir.undefined !fir.dscope
+    %2:2 = fir.unboxchar %0 : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)  loc(#loc1)
+    %3 = fircg.ext_declare %2#0 typeparams %2#1 dummy_scope %1 {uniq_name = "_QFtestEstr"} : (!fir.ref<!fir.char<1,?>>, index, !fir.dscope) -> !fir.ref<!fir.char<1,?>> loc(#loc1)
+    return
+  } loc(#loc2)
+}
+
+#loc1 = loc("test.f90":5:1)
+#loc2 = loc("test.f90":15:1)
+
+
+// CHECK: fir.unboxchar{{.*}} loc(#[[LOC1:.*]])
+// CHECK: #[[LOC2:.*]] = loc("test.f90":5:1)
+// CHECK: #[[VAR:.*]] = #llvm.di_local_variable<{{.*}}name = "._QFtestEstr"{{.*}}flags = Artificial>
+// CHECK: #[[STR_TY:.*]] = #llvm.di_string_type<tag = DW_TAG_string_type, name = "", stringLength = #[[VAR]], encoding = DW_ATE_ASCII>
+// CHECK: #[[LOC1]] = loc(fused<#di_local_variable>[#[[LOC2]]])
+// CHECK: #llvm.di_local_variable<{{.*}}name = "str"{{.*}}type = #[[STR_TY]]>
+
+// CODEGEN: #[[VAR1:.*]] = #llvm.di_local_variable<{{.*}}name = "._QFtestEstr"{{.*}}flags = Artificial>
+// CODEGEN: llvm.func @test
+// CODEGEN: llvm.intr.dbg.value #[[VAR1]]