[flang][debug] Add support for common blocks.

[abidh]

This PR adds debug support for common block in flang. As variable which are part of a common block don't have a special marker to recognize them, we use the following check to find them.

%0 = fir.address_of(@A)
%1 = fir.convert %0
%2 = fir.coordinate_of %1, %c0
%3 = fir.convert %2
%4 = fircg.ext_declare %3

If the memref of a fircg.ext_declare points to a fir.coordinate_of and that in turn points to an fir.address_of (ignoring immediate fir.convert) then we assume that it is a common block variable. The fir.address_of gives us the global symbol which is the storage for common block and fir.coordinate_of provides the offset in this storage.

The debug hierarchy looks like as

subroutine f3
integer :: x, y
common /a/ x, y
end subroutine

@a_ = global { ... } { ... }, !dbg !26, !dbg !28

!23 = !DISubprogram(name: "f3"...)
!24 = !DICommonBlock(scope: !23, name: "a", ...)
!25 = !DIGlobalVariable(name: "x", scope: !24 ...)
!26 = !DIGlobalVariableExpression(var: !25, expr: !DIExpression())
!27 = !DIGlobalVariable(name: "y", scope: !24 ...)
!28 = !DIGlobalVariableExpression(var: !27, expr: !DIExpression(DW_OP_plus_uconst, 4))

This required following changes:

1. Instead of using DIGlobalVariableAttr in the FusedLoc of GlobalOp, we use DIGlobalVariableExpressionAttr. This allows us the generate the DIExpression where we have the information.

2. Previously, only one DIGlobalVariableExpressionAttr could be linked to one global op. I recently removed this restriction in mlir. To make use of it, we add an ArrayAttr to the FusedLoc of a GlobalOp. This allows us to pass multiple DIGlobalVariableExpressionAttr.

3. I was depending on the name of global for the name of the common block. The name gets a '_' appended. I could not find a utility function in flang to remove it so I have to brute force it.

[llvmbot]

@llvm/pr-subscribers-flang-codegen

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

Author: Abid Qadeer (abidh)

Changes

This PR adds debug support for common block in flang. As variable which are part of a common block don't have a special marker to recognize them, we use the following check to find them.

%0 = fir.address_of(@a)
%1 = fir.convert %0
%2 = fir.coordinate_of %1, %c0
%3 = fir.convert %2
%4 = fircg.ext_declare %3

If the memref of a fircg.ext_declare points to a fir.coordinate_of and that in turn points to an fir.address_of (ignoring immediate fir.convert) then we assume that it is a common block variable. The fir.address_of gives us the global symbol which is the storage for common block and fir.coordinate_of provides the offset in this storage.

The debug hierarchy looks like as

subroutine f3
integer :: x, y
common /a/ x, y
end subroutine

@a_ = global { ... } { ... }, !dbg !26, !dbg !28

!23 = !DISubprogram(name: "f3"...)
!24 = !DICommonBlock(scope: !23, name: "a", ...)
!25 = !DIGlobalVariable(name: "x", scope: !24 ...)
!26 = !DIGlobalVariableExpression(var: !25, expr: !DIExpression())
!27 = !DIGlobalVariable(name: "y", scope: !24 ...)
!28 = !DIGlobalVariableExpression(var: !27, expr: !DIExpression(DW_OP_plus_uconst, 4))

This required following changes:

1. Instead of using DIGlobalVariableAttr in the FusedLoc of GlobalOp, we use DIGlobalVariableExpressionAttr. This allows us the generate the DIExpression where we have the information.

2. Previously, only one DIGlobalVariableExpressionAttr could be linked to one global op. I recently removed this restriction in mlir. To make use of it, we add an ArrayAttr to the FusedLoc of a GlobalOp. This allows us to pass multiple DIGlobalVariableExpressionAttr.

3. I was depending on the name of global for the name of the common block. The name gets a '_' appended. I could not find a utility function in flang to remove it so I have to brute force it.

---

Patch is 34.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112398.diff

6 Files Affected:

• (modified) flang/lib/Optimizer/CodeGen/CodeGen.cpp (+4-5)
• (modified) flang/lib/Optimizer/Transforms/AddDebugInfo.cpp (+113-1)
• (added) flang/test/Integration/debug-common-block-1.f90 (+138)
• (added) flang/test/Transforms/debug-common-block.fir (+213)
• (modified) flang/test/Transforms/debug-module-1.fir (+4-2)
• (modified) flang/test/Transforms/debug-module-2.fir (+4-2)

diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index 9b624efa053813..0d9b7a2b86f853 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -2811,11 +2811,10 @@ struct GlobalOpConversion : public fir::FIROpConversion<fir::GlobalOp> {
     llvm::SmallVector<mlir::Attribute> dbgExprs;
 
     if (auto fusedLoc = mlir::dyn_cast<mlir::FusedLoc>(global.getLoc())) {
-      if (auto gvAttr =
-              mlir::dyn_cast_or_null<mlir::LLVM::DIGlobalVariableAttr>(
-                  fusedLoc.getMetadata())) {
-        dbgExprs.push_back(mlir::LLVM::DIGlobalVariableExpressionAttr::get(
-            global.getContext(), gvAttr, mlir::LLVM::DIExpressionAttr()));
+      if (auto gvExprAttr = mlir::dyn_cast_if_present<mlir::ArrayAttr>(
+              fusedLoc.getMetadata())) {
+        for (auto attr : gvExprAttr.getAsRange<mlir::Attribute>())
+          dbgExprs.push_back(attr);
       }
     }
 
diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index 400a8648dd7e07..208d67d973cc25 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -59,10 +59,19 @@ class AddDebugInfoPass : public fir::impl::AddDebugInfoBase<AddDebugInfoPass> {
 
 private:
   llvm::StringMap<mlir::LLVM::DIModuleAttr> moduleMap;
+  llvm::StringMap<mlir::LLVM::DICommonBlockAttr> commonBlockMap;
+  // List of GlobalVariableExpressionAttr that are attached to a given global
+  // that represents the storage for common block.
+  llvm::DenseMap<fir::GlobalOp, llvm::SmallVector<mlir::Attribute>>
+      globalToGlobalExprsMap;
 
   mlir::LLVM::DIModuleAttr getOrCreateModuleAttr(
       const std::string &name, mlir::LLVM::DIFileAttr fileAttr,
       mlir::LLVM::DIScopeAttr scope, unsigned line, bool decl);
+  mlir::LLVM::DICommonBlockAttr
+  getOrCreateCommonBlockAttr(const std::string &name,
+                             mlir::LLVM::DIFileAttr fileAttr,
+                             mlir::LLVM::DIScopeAttr scope, unsigned line);
 
   void handleGlobalOp(fir::GlobalOp glocalOp, mlir::LLVM::DIFileAttr fileAttr,
                       mlir::LLVM::DIScopeAttr scope,
@@ -73,6 +82,12 @@ class AddDebugInfoPass : public fir::impl::AddDebugInfoBase<AddDebugInfoPass> {
                     mlir::LLVM::DICompileUnitAttr cuAttr,
                     fir::DebugTypeGenerator &typeGen,
                     mlir::SymbolTable *symbolTable);
+  bool createCommonBlockGlobal(fir::cg::XDeclareOp declOp,
+                               const std::string &name,
+                               mlir::LLVM::DIFileAttr fileAttr,
+                               mlir::LLVM::DIScopeAttr scopeAttr,
+                               fir::DebugTypeGenerator &typeGen,
+                               mlir::SymbolTable *symbolTable);
   std::optional<mlir::LLVM::DIModuleAttr>
   getModuleAttrFromGlobalOp(fir::GlobalOp globalOp,
                             mlir::LLVM::DIFileAttr fileAttr,
@@ -90,6 +105,67 @@ bool debugInfoIsAlreadySet(mlir::Location loc) {
 
 } // namespace
 
+bool AddDebugInfoPass::createCommonBlockGlobal(
+    fir::cg::XDeclareOp declOp, const std::string &name,
+    mlir::LLVM::DIFileAttr fileAttr, mlir::LLVM::DIScopeAttr scopeAttr,
+    fir::DebugTypeGenerator &typeGen, mlir::SymbolTable *symbolTable) {
+  mlir::MLIRContext *context = &getContext();
+  mlir::OpBuilder builder(context);
+  std::optional<std::int64_t> optint;
+  auto op = declOp.getMemref().getDefiningOp();
+
+  if (auto conOp = mlir::dyn_cast_if_present<fir::ConvertOp>(op))
+    op = conOp.getValue().getDefiningOp();
+
+  if (auto cordOp = mlir::dyn_cast_if_present<fir::CoordinateOp>(op)) {
+    optint = fir::getIntIfConstant(cordOp.getOperand(1));
+    if (!optint)
+      return false;
+    op = cordOp.getRef().getDefiningOp();
+    if (auto conOp2 = mlir::dyn_cast_if_present<fir::ConvertOp>(op))
+      op = conOp2.getValue().getDefiningOp();
+
+    if (auto addrOfOp = mlir::dyn_cast_if_present<fir::AddrOfOp>(op)) {
+      auto sym = addrOfOp.getSymbol();
+      if (auto global =
+              symbolTable->lookup<fir::GlobalOp>(sym.getRootReference())) {
+
+        unsigned line = getLineFromLoc(global.getLoc());
+        std::string commonName(sym.getRootReference().str());
+        // FIXME: We are trying to extract the name of the common block from the
+        // name of the global. As part of mangling, GetCommonBlockObjectName can
+        // add a trailing _ in the name of that global. The demangle function
+        // does not seem to handle such cases. So the following hack is used to
+        // remove the trailing '_'.
+        if (commonName != Fortran::common::blankCommonObjectName &&
+            commonName.back() == '_')
+          commonName.pop_back();
+        mlir::LLVM::DICommonBlockAttr commonBlock =
+            getOrCreateCommonBlockAttr(commonName, fileAttr, scopeAttr, line);
+        mlir::LLVM::DITypeAttr diType = typeGen.convertType(
+            fir::unwrapRefType(declOp.getType()), fileAttr, scopeAttr, declOp);
+        line = getLineFromLoc(declOp.getLoc());
+        auto gvAttr = mlir::LLVM::DIGlobalVariableAttr::get(
+            context, commonBlock, mlir::StringAttr::get(context, name),
+            declOp.getUniqName(), fileAttr, line, diType,
+            /*isLocalToUnit*/ true, /*isDefinition*/ true, /* alignInBits*/ 0);
+        mlir::LLVM::DIExpressionAttr expr;
+        if (*optint != 0) {
+          llvm::SmallVector<mlir::LLVM::DIExpressionElemAttr> ops;
+          ops.push_back(mlir::LLVM::DIExpressionElemAttr::get(
+              context, llvm::dwarf::DW_OP_plus_uconst, *optint));
+          expr = mlir::LLVM::DIExpressionAttr::get(context, ops);
+        }
+        auto dbgExpr = mlir::LLVM::DIGlobalVariableExpressionAttr::get(
+            global.getContext(), gvAttr, expr);
+        globalToGlobalExprsMap[global].push_back(dbgExpr);
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 void AddDebugInfoPass::handleDeclareOp(fir::cg::XDeclareOp declOp,
                                        mlir::LLVM::DIFileAttr fileAttr,
                                        mlir::LLVM::DIScopeAttr scopeAttr,
@@ -101,6 +177,11 @@ void AddDebugInfoPass::handleDeclareOp(fir::cg::XDeclareOp declOp,
 
   if (result.first != fir::NameUniquer::NameKind::VARIABLE)
     return;
+
+  if (createCommonBlockGlobal(declOp, result.second.name, fileAttr, scopeAttr,
+                              typeGen, symbolTable))
+    return;
+
   // If this DeclareOp actually represents a global then treat it as such.
   if (auto global = symbolTable->lookup<fir::GlobalOp>(declOp.getUniqName())) {
     handleGlobalOp(global, fileAttr, scopeAttr, typeGen, symbolTable, declOp);
@@ -136,6 +217,22 @@ void AddDebugInfoPass::handleDeclareOp(fir::cg::XDeclareOp declOp,
   declOp->setLoc(builder.getFusedLoc({declOp->getLoc()}, localVarAttr));
 }
 
+mlir::LLVM::DICommonBlockAttr AddDebugInfoPass::getOrCreateCommonBlockAttr(
+    const std::string &name, mlir::LLVM::DIFileAttr fileAttr,
+    mlir::LLVM::DIScopeAttr scope, unsigned line) {
+  mlir::MLIRContext *context = &getContext();
+  mlir::LLVM::DICommonBlockAttr cbAttr;
+  if (auto iter{commonBlockMap.find(name)}; iter != commonBlockMap.end()) {
+    cbAttr = iter->getValue();
+  } else {
+    cbAttr = mlir::LLVM::DICommonBlockAttr::get(
+        context, scope, nullptr, mlir::StringAttr::get(context, name), fileAttr,
+        line);
+    commonBlockMap[name] = cbAttr;
+  }
+  return cbAttr;
+}
+
 // The `module` does not have a first class representation in the `FIR`. We
 // extract information about it from the name of the identifiers and keep a
 // map to avoid duplication.
@@ -230,7 +327,10 @@ void AddDebugInfoPass::handleGlobalOp(fir::GlobalOp globalOp,
       mlir::StringAttr::get(context, globalOp.getName()), fileAttr, line,
       diType, /*isLocalToUnit*/ false,
       /*isDefinition*/ globalOp.isInitialized(), /* alignInBits*/ 0);
-  globalOp->setLoc(builder.getFusedLoc({globalOp->getLoc()}, gvAttr));
+  auto dbgExpr = mlir::LLVM::DIGlobalVariableExpressionAttr::get(
+      globalOp.getContext(), gvAttr, nullptr);
+  auto arrayAttr = mlir::ArrayAttr::get(context, {dbgExpr});
+  globalOp->setLoc(builder.getFusedLoc({globalOp.getLoc()}, arrayAttr));
 }
 
 void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
@@ -412,6 +512,11 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
     if (&funcOp.front() == declOp->getBlock())
       handleDeclareOp(declOp, fileAttr, spAttr, typeGen, symbolTable);
   });
+  // commonBlockMap ensures that we don't create multiple DICommonBlockAttr of
+  // the same name in one function. But it is ok (rather required) to create
+  // them in different functions if common block of the same name has been used
+  // there.
+  commonBlockMap.clear();
 }
 
 void AddDebugInfoPass::runOnOperation() {
@@ -464,6 +569,13 @@ void AddDebugInfoPass::runOnOperation() {
   module.walk([&](mlir::func::FuncOp funcOp) {
     handleFuncOp(funcOp, fileAttr, cuAttr, typeGen, &symbolTable);
   });
+  mlir::OpBuilder builder(context);
+  // We have processed all function. Attach common block variables to the
+  // global that represent the storage.
+  for (auto [global, exprs] : globalToGlobalExprsMap) {
+    auto arrayAttr = mlir::ArrayAttr::get(context, exprs);
+    global->setLoc(builder.getFusedLoc({global.getLoc()}, arrayAttr));
+  }
   // Process any global which was not processed through DeclareOp.
   if (debugLevel == mlir::LLVM::DIEmissionKind::Full) {
     // Process 'GlobalOp' only if full debug info is requested.
diff --git a/flang/test/Integration/debug-common-block-1.f90 b/flang/test/Integration/debug-common-block-1.f90
new file mode 100644
index 00000000000000..18217637be0fa1
--- /dev/null
+++ b/flang/test/Integration/debug-common-block-1.f90
@@ -0,0 +1,138 @@
+! RUN: %flang_fc1 -emit-llvm -debug-info-kind=standalone %s -o - | FileCheck  %s
+
+subroutine f1
+  real(kind=4) :: x, y, xa, ya
+  common // x, y
+  common /a/ xa, ya
+  x = 1.1
+  y = 2.2
+  xa = 3.3
+  ya = 4.4
+  print *, x, y, xa, ya
+end subroutine
+! CHECK-DAG: ![[XF1:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "x", linkageName: "_QFf1Ex", scope: ![[CBF1:[0-9]+]], file: !5, line: [[@LINE-9]], type: ![[REAL:[0-9]+]]{{.*}})
+! CHECK-DAG: ![[EXPXF1:[0-9]+]] = !DIGlobalVariableExpression(var: ![[XF1]], expr: !DIExpression())
+! CHECK-DAG: ![[YF1:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "y", linkageName: "_QFf1Ey", scope: ![[CBF1]], file: !{{[0-9]+}}, line: [[@LINE-11]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPYF1:[0-9]+]] = !DIGlobalVariableExpression(var: ![[YF1]], expr: !DIExpression(DW_OP_plus_uconst, 4))
+! CHECK-DAG: ![[XAF1:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "xa", linkageName: "_QFf1Exa", scope: ![[CBAF1:[0-9]+]], file: !{{[0-9]+}}, line: [[@LINE-13]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPXAF1:[0-9]+]] = !DIGlobalVariableExpression(var: ![[XAF1]], expr: !DIExpression())
+! CHECK-DAG: ![[YAF1:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "ya", linkageName: "_QFf1Eya", scope: ![[CBAF1]], file: !{{[0-9]+}}, line: [[@LINE-15]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPYAF1:[0-9]+]] = !DIGlobalVariableExpression(var: ![[YAF1]], expr: !DIExpression(DW_OP_plus_uconst, 4))
+
+
+subroutine f2
+  real(kind=4) :: x, y, z, xa, ya, za
+  common // x, y, z
+  common /a/ xa, ya, za
+  print *, x, y, z, xa, ya, za
+end subroutine
+! CHECK-DAG: ![[XF2:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "x", linkageName: "_QFf2Ex", scope: ![[CBF2:[0-9]+]], file: !{{[0-9]+}}, line: [[@LINE-5]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPXF2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[XF2]], expr: !DIExpression())
+! CHECK-DAG: ![[YF2:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "y", linkageName: "_QFf2Ey", scope: ![[CBF2]], file: !{{[0-9]+}}, line: [[@LINE-7]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPYF2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[YF2]], expr: !DIExpression(DW_OP_plus_uconst, 4))
+! CHECK-DAG: ![[ZF2:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "z", linkageName: "_QFf2Ez", scope: ![[CBF2]], file: !{{[0-9]+}}, line: [[@LINE-9]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPZF2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[ZF2]], expr: !DIExpression(DW_OP_plus_uconst, 8))
+! CHECK-DAG: ![[XAF2:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "xa", linkageName: "_QFf2Exa", scope: ![[CBAF2:[0-9]+]], file: !{{[0-9]+}}, line: [[@LINE-11]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPXAF2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[XAF2]], expr: !DIExpression())
+! CHECK-DAG: ![[YAF2:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "ya", linkageName: "_QFf2Eya", scope: ![[CBAF2]], file: !{{[0-9]+}}, line: [[@LINE-13]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPYAF2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[YAF2]], expr: !DIExpression(DW_OP_plus_uconst, 4))
+! CHECK-DAG: ![[ZAF2:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "za", linkageName: "_QFf2Eza", scope: ![[CBAF2]], file: !{{[0-9]+}}, line: [[@LINE-15]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPZAF2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[ZAF2]], expr: !DIExpression(DW_OP_plus_uconst, 8))
+
+subroutine f3
+  integer(kind=4) :: x = 42, xa = 42
+  common // x
+  common /a/ xa
+  print *, x
+  print *, xa
+end subroutine
+! CHECK-DAG: ![[XF3:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "x", linkageName: "_QFf3Ex", scope: ![[CBF3:[0-9]+]], file: !{{[0-9]+}}, line: [[@LINE-6]], type: ![[INT:[0-9]+]]{{.*}})
+! CHECK-DAG: ![[EXPXF3:[0-9]+]] = !DIGlobalVariableExpression(var: ![[XF3]], expr: !DIExpression())
+! CHECK-DAG: ![[XAF3:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "xa", linkageName: "_QFf3Exa", scope: ![[CBAF3:[0-9]+]], file: !{{[0-9]+}}, line: [[@LINE-8]], type: ![[INT]]{{.*}})
+! CHECK-DAG: ![[EXPXAF3:[0-9]+]] = !DIGlobalVariableExpression(var: ![[XAF3]], expr: !DIExpression())
+
+program test
+  real(kind=4) :: v1, v2, v3, va1, va2, va3
+  common // v1, v2, v3
+  common /a/ va1, va2, va3
+  call f1()
+  call f2()
+  call f3()
+  print *, v1, va1, va3
+END
+! CHECK-DAG: ![[V1:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "v1", linkageName: "_QFEv1", scope: ![[CBM:[0-9]+]], file: !{{[0-9]+}}, line: [[@LINE-8]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPV1:[0-9]+]] = !DIGlobalVariableExpression(var: ![[V1]], expr: !DIExpression())
+! CHECK-DAG: ![[V2:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "v2", linkageName: "_QFEv2", scope: ![[CBM]], file: !{{[0-9]+}}, line: [[@LINE-10]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPV2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[V2]], expr: !DIExpression(DW_OP_plus_uconst, 4))
+! CHECK-DAG: ![[V3:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "v3", linkageName: "_QFEv3", scope: ![[CBM]], file: !{{[0-9]+}}, line: [[@LINE-12]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPV3:[0-9]+]] = !DIGlobalVariableExpression(var: ![[V3]], expr: !DIExpression(DW_OP_plus_uconst, 8))
+! CHECK-DAG: ![[VA1:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "va1", linkageName: "_QFEva1", scope: ![[CBAM:[0-9]+]], file: !{{[0-9]+}}, line: [[@LINE-14]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPVA1:[0-9]+]] = !DIGlobalVariableExpression(var: ![[VA1]], expr: !DIExpression())
+! CHECK-DAG: ![[VA2:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "va2", linkageName: "_QFEva2", scope: ![[CBAM]], file: !{{[0-9]+}}, line: [[@LINE-16]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPVA2:[0-9]+]] = !DIGlobalVariableExpression(var: ![[VA2]], expr: !DIExpression(DW_OP_plus_uconst, 4))
+! CHECK-DAG: ![[VA3:[0-9]+]] = {{.*}}!DIGlobalVariable(name: "va3", linkageName: "_QFEva3", scope: ![[CBAM]], file: !{{[0-9]+}}, line: [[@LINE-18]], type: ![[REAL]]{{.*}})
+! CHECK-DAG: ![[EXPVA3:[0-9]+]] = !DIGlobalVariableExpression(var: ![[VA3]], expr: !DIExpression(DW_OP_plus_uconst, 8))
+
+
+! CHECK-DAG: ![[REAL]] = !DIBasicType(name: "real", size: 32, encoding: DW_ATE_float)
+! CHECK-DAG: ![[INT]] = !DIBasicType(name: "integer", size: 32, encoding: DW_ATE_signed)
+
+! CHECK-DAG: ![[F1:[0-9]+]] = {{.*}}!DISubprogram(name: "f1"{{.*}})
+! CHECK-DAG: ![[CBF1]] = !DICommonBlock(scope: ![[F1]], declaration: null, name: "__BLNK__"{{.*}})
+! CHECK-DAG: ![[CBAF1]] = !DICommonBlock(scope: ![[F1]], declaration: null, name: "a"{{.*}})
+
+! CHECK-DAG: ![[F2:[0-9]+]] = {{.*}}!DISubprogram(name: "f2"{{.*}})
+! CHECK-DAG: ![[CBF2]] = !DICommonBlock(scope: ![[F2]], declaration: null, name: "__BLNK__"{{.*}})
+! CHECK-DAG: ![[CBAF2]] = !DICommonBlock(scope: ![[F2]], declaration: null, name: "a"{{.*}})
+
+! CHECK-DAG: ![[F3:[0-9]+]] = {{.*}}!DISubprogram(name: "f3"{{.*}})
+! CHECK-DAG: ![[CBF3]] = !DICommonBlock(scope: ![[F3]], declaration: null, name: "__BLNK__"{{.*}})
+! CHECK-DAG: ![[CBAF3]] = !DICommonBlock(scope: ![[F3]], declaration: null, name: "a"{{.*}})
+
+! CHECK-DAG: ![[MAIN:[0-9]+]] = {{.*}}!DISubprogram(name: "test"{{.*}})
+! CHECK-DAG: ![[CBM]] = !DICommonBlock(scope: ![[MAIN]], declaration: null, name: "__BLNK__"{{.*}})
+! CHECK-DAG: ![[CBAM]] = !DICommonBlock(scope: ![[MAIN]], declaration: null, name: "a"{{.*}})
+
+! Using CHECK-DAG-SAME so that we are not dependent on order of variable in these lists.
+! CHECK-DAG: @__BLNK__ = global{{.*}}
+! CHECK-DAG-SAME: !dbg ![[EXPXF1]]
+! CHECK-DAG-SAME: !dbg ![[EXPYF1]]
+! CHECK-DAG-SAME: !dbg ![[EXPXF2]]
+! CHECK-DAG-SAME: !dbg ![[EXPYF2]]
+! CHECK-DAG-SAME: !dbg ![[EXPZF2]]
+! CHECK-DAG-SAME: !dbg ![[EXPXF3]]
+! CHECK-DAG-SAME: !dbg ![[EXPV1]]
+! CHECK-DAG-SAME: !dbg ![[EXPV2]]
+! CHECK-DAG-SAME: !dbg ![[EXPV3]]
+
+! CHECK-DAG: @a_ = global{{.*}}
+! CHECK-DAG-SAME: !dbg ![[EXPXAF1]]
+! CHECK-DAG-SAME: !dbg ![[EXPYAF1]]
+! CHECK-DAG-SAME: !dbg ![[EXPXAF2]]
+! CHECK-DAG-SAME: !dbg ![[EXPYAF2]]
+! CHECK-DAG-SAME: !dbg ![[EXPZAF2]]
+! CHECK-DAG-SAME: !dbg ![[EXPXAF3]]
+! CHECK-DAG-SAME: !dbg ![[EXPVA1]]
+! CHECK-DAG-SAME: !dbg ![[EXPVA2]]
+! CHECK-DAG-SAME: !dbg ![[EXPVA3]]
+
+! CHECK-DAG: !DICompileUnit({{.*}}, globals: ![[GLOBALS:[0-9]+]])
+! CHECK-DAG: ![[GLOBALS]]
+! CHECK-DAG-SAME: ![[EXPXF1]]
+! CHECK-DAG-SAME: ![[EXPYF1]]
+! CHECK-DAG-SAME: ![[EXPXAF1]]
+! CHECK-DAG-SAME: ![[EXPYAF1]]
+! CHECK-DAG-SAME: ![[EXPXF2]]
+! CHECK-DAG-SAME: ![[EXPYF2]]
+! CHECK-DAG-SAME: ![[EXPZF2]]
+! CHECK-DAG-SAME: ![[EXPXAF2]]
+! CHECK-DAG-SAME: ![[EXPYAF2]]
+! CHECK-DAG-SAME: ![[EXPZAF2]]
+! CHECK-DAG-SAME: ![[EXPXF3]]
+! CHECK-DAG-SAME: ![[EXPXAF3]]
+! CHECK-DAG-SAME: ![[EXPV1]]
+! CHECK-DAG-SAME: ![[EXPV2]]
+! CHECK-DAG-SAME: ![[EXPV3]]
+! CHECK-DAG-SAME: ![[EXPVA1]]
+! CHECK-DAG-SAME: ![[EXPVA2]]
+! CHECK-DAG-SAME: ![[EXPVA3]]
diff --git a/flang/test/Transforms/debug-common-block.fir b/flang/test/Transforms/debug-common-block.fir
new file mode 100644
index 00000000000000..481b26369a92ce
--- /dev/null
+++ b/flang/test/Transforms/debug-common-block.fir
@@ -0,0 +1,213 @@
+// RUN: fir-opt --add-debug-info --mlir-print-debuginfo %s | FileCheck %s
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<>} {
+  fir.global @__BLNK__ {alignment = 4 : i64} : tuple<i32, !fir.array<8xi8>> {} loc(#loc1)
+  fir.global @a_ {alignment = 4 : i64} : tuple<i32, !fir.array<8xi8>> {} loc(#loc2)
+  func.func @f1() {
+    %c9_i32 = arith.constant 9 : i32
+    %c6_i32 = arith.constant 6 : i32
+    %cst = arith.constant 4.400000e+00 : f32
+    %cst_0 = arith.constant 3.300000e+00 : f32
+    %cst_1 = arith.constant 2.200000e+00 : f32
+    %cst_2 = arith.constant 1.100000e+00 : f32
+    %c4 = arith.constant 4 : index
+    %c0 = arith.constant 0 : index
+    %0 = fir.address_of(@__BLNK__) : !fir.ref<tuple<i32, !fir.array<8xi8>>>
+    %1 = fir.convert %0 : (!fir.ref<tuple<i32, !fir.array<8xi8>>>) -> !fir.ref<!fir.array<?xi8>>
+    %2 = fir.coordinate_of %1, %c0 : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+    %3 = fir.convert %2 : (!fir.ref<i8>) -> !fir.ref<f32>
+    %4 = fircg.ext_declare %3 {uniq_name = "_QFf1Ex"} : (!fir.ref<f32>) -> !fir.ref<f32> loc(#loc4)
+    %5 = fir.address_of(@a_) : !fir.ref<tuple<i32, !fir.array<8xi8>>>
+    %6 = fir.convert %5 : (!fir.ref<tuple<i32, !fir.array<8xi8>>>) -> !fir.ref<!fir.array<?xi8>>
+    %7 = fir.coordinate_of %6, %c0 : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+    %8 = fir.convert %7 : (!fir.ref<i8>) -> !fir.ref<f32>
+    %9 = fircg.ext_declare %8 {uniq_name = "_QFf1Exa"} : (!fir.ref<f32>) -> !fir.ref<f32> loc(#loc5)
+    %10 = fir.coordinate_of %1, %c4 : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+    %11 = fir.convert %10 : ...
[truncated]

[tblah]

I think you could make the detection more robust by ensuring that the linkage attribute of the fir.global is "common".

[tblah]

ultra-nit: Creating a std::string leads to unnecessary heap allocation. A SymbolRefAttr can also be converted into an llvm::StringRef, which can then be used as an argument type to getOrCreateCommonBlockAttr.

StringRef::drop_back just creates a StringRef with a shorter length. It does not modify the source buffer.

[abidh]

Thanks for the suggestion. I have used llvm::StringRef now.

[tblah]

nit: Usual flang style is to spell out types where these are not stated in the initializer for the variable.

For example, auto sym = addrOfOp.getSymbol() should spell out the type of sym, but auto conOp = mlir::dyn_cast_if_present<fir::convertOp>(op) already makes the type clear.

[abidh]

Thanks for the catch. Fixed now.

[abidh]

I think you could make the detection more robust by ensuring that the linkage attribute of the fir.global is "common".

I actually had this check but later noticed during my testing that sometime that global would not have "common" linkage. IIRC it was when common block is declared inside a module.

[tblah]

I think you could make the detection more robust by ensuring that the linkage attribute of the fir.global is "common".

I actually had this check but later noticed during my testing that sometime that global would not have "common" linkage. IIRC it was when common block is declared inside a module.

Ahh good catch. My concern is that operation sequence you are matching looks quite generic. I wonder if we should add a unit attribute to fir.global which indicates whether it comes from a common block and set that during lowering?

[jeanPerier]

I actually had this check but later noticed during my testing that sometime that global would not have "common" linkage. IIRC it was when common block is declared inside a module.

Can you give an example, I think a common block global without common linkage is most likely a bug (which is possible).

However, I would not push on assuming that common linkage imply common block, we may want to use that in other cases. What about adding an attribute on the global instead with the common block name so that you do not have to care about the assembly name, like fir.common_block = "a"?

I also feel like adding an attribute to the declare with the offset and global name may be a lot more robust than pattern matching the computation (something like fir.declare ...... {fir.in_common_block = "a", fir.common_block_offset = 12}) would be cleaner. It duplicate information a bit, but I think retrieving information in DAGs outside of optimization purposes is quickly brittle.

If those option sounds good to you, I am happy to extend FIR here.

[abidh]

I actually had this check but later noticed during my testing that sometime that global would not have "common" linkage. IIRC it was when common block is declared inside a module.

Can you give an example, I think a common block global without common linkage is most likely a bug (which is possible).

However, I would not push on assuming that common linkage imply common block, we may want to use that in other cases. What about adding an attribute on the global instead with the common block name so that you do not have to care about the assembly name, like fir.common_block = "a"?

Here is the example code and the generated fir.

module helper
  implicit none
  integer :: x = 42, xa = 52
  common /a/ x, xa
end module helper


  fir.global @a_ {alignment = 4 : i64} : tuple<i32, i32> {
    %0 = fir.zero_bits tuple<i32, i32>
    %c42_i32 = arith.constant 42 : i32
    %1 = fir.insert_value %0, %c42_i32, [0 : index] : (tuple<i32, i32>, i32) -> tuple<i32, i32>
    %c52_i32 = arith.constant 52 : i32
    %2 = fir.insert_value %1, %c52_i32, [1 : index] : (tuple<i32, i32>, i32) -> tuple<i32, i32>
    fir.has_value %2 : tuple<i32, i32>
  }

I also feel like adding an attribute to the declare with the offset and global name may be a lot more robust than pattern matching the computation (something like fir.declare ...... {fir.in_common_block = "a", fir.common_block_offset = 12}) would be cleaner. It duplicate information a bit, but I think retrieving information in DAGs outside of optimization purposes is quickly brittle.

If those option sounds good to you, I am happy to extend FIR here.

I will agree to that. I had this concern that we are depending on pattern matching which is quite fragile. Having this information in IR would be great. Another item on my wish list is having the position of the argument to function/subroutine on the declare op which is will save us from doing similar pattern matching for CHARACTER type arguments.

[jinisusan]

A couple of comments:

• It might be good to have the common block variables tagged with the "DW_AT_external" attribute in line with what is done by other fortran compilers.
• Also, it might be good to include the linkage name attribute while generating DICommonBlock

[jiel-nv]

Hi @abidh, I'm visiting this PR and wondering if you would like to share your plan about how to proceed with this support for common blocks? Is the FIR extension that @jeanPerier suggested a prerequiste?

[abidh]

Hi @abidh, I'm visiting this PR and wondering if you would like to share your plan about how to proceed with this support for common blocks? Is the FIR extension that @jeanPerier suggested a prerequiste?

Hi @jieljiel

The FIR extension is the best way to detect the common block but the current approach also works. I was wondering that we can use this PR for now as it fills a missing hole in the debug support. When the extension in FIR to detect common block is available in future, we can use that and remove the DAG walking. If @jeanPerier thinks this is OK then I can rebase/update the PR and address any other comments.

[jiel-nv]

Hi @abidh, I'm visiting this PR and wondering if you would like to share your plan about how to proceed with this support for common blocks? Is the FIR extension that @jeanPerier suggested a prerequiste?

Hi @jieljiel

The FIR extension is the best way to detect the common block but the current approach also works. I was wondering that we can use this PR for now as it fills a missing hole in the debug support. When the extension in FIR to detect common block is available in future, we can use that and remove the DAG walking. If @jeanPerier thinks this is OK then I can rebase/update the PR and address any other comments.

@jeanPerier Would you mind letting @abidh 's PR in and then he will make adjustment based on FIR extensions later.

@abidh Would you be able to implement the required FIR extension, or you need @jeanPerier 's help working on that?

[jeanPerier]

@jeanPerier Would you mind letting @abidh 's PR in and then he will make adjustment based on FIR extensions later.

Yes, you can go ahead and update/merge this and I will ping you when I have the FIR changes.

[jeanPerier]

@jeanPerier Would you mind letting @abidh 's PR in and then he will make adjustment based on FIR extensions later.

Yes, you can go ahead and update/merge this and I will ping you when I have the FIR changes.

[jeanPerier]

Can you give an example, I think a common block global without common linkage is most likely a bug (which is possible).
Here is the example code and the generated fir.

I never replied to that, thanks for the example. It is actually correct and expected to not give the "common" attribute to global symbols for common block with an initial value because it is the definition of the symbol (in other compilation units, it cannot be given initial values and will have the common linkage).

LLVM IR actually forbids common linkage on a global with a non zero initial value (from LLVM ref manual: common symbols may not have an explicit section, must have a zero initializer, and may not be marked constant.)

[abidh]

@jeanPerier Would you mind letting @abidh 's PR in and then he will make adjustment based on FIR extensions later.

Yes, you can go ahead and update/merge this and I will ping you when I have the FIR changes.

I have rebased the PR. I made a small change to address the comments that DW_AT_external should true for these variables in DWARF as it is in other compilers. Please let me know if you have any further comments.

[abidh]

@jeanPerier Would you mind letting @abidh 's PR in and then he will make adjustment based on FIR extensions later.

Yes, you can go ahead and update/merge this and I will ping you when I have the FIR changes.

Hi @jeanPerier
Your comment suggested that this PR is ok to go in. Can you kindly approve if it is still the case. Thanks.

[jeanPerier]

Small question, LGTM otherwise, thanks.

[jeanPerier]

Should there be a if(auto dbgAttr = mlir::dyn_cast<DIGlobalVariableExpressionAttr>(attr)) to ensure these is indeed debug info?

[abidh]

Although only DIGlobalVariableExpressionAttr are added in ArrayAttr at the moment but I guess it is better to have this check to be safe. I will add it before merge. Please also see my comments here on why this interface is relatively less type strict.

[jeanPerier]

Thanks for the change and link.