Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang] Add debug information for module variables. #91582

Merged
merged 9 commits into from
May 22, 2024
Merged

Conversation

abidh
Copy link
Contributor

@abidh abidh commented May 9, 2024

This PR add debug info for module variables. The module variables are added as global variables but their scope is set to module instead of compile unit. The scope of function declared inside a module is also set accordingly.

After this patch, a module variable could be evaluated in the GDB as p helper::gli where helper is name of the module and gli is the name of the variable. A future patch will add the import module functionality which will remove the need to prefix the name with helper::.

The line number where is module is declared is a best guess at the moment as this information is not part of the GlobalOp.

@llvmbot
Copy link
Collaborator

llvmbot commented May 9, 2024

@llvm/pr-subscribers-mlir-llvm
@llvm/pr-subscribers-mlir

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

Author: Abid Qadeer (abidh)

Changes

This PR add debug info for module variables. The module variables are added as global variables but their scope is set to module instead of compile unit. The scope of function declared inside a module is also set accordingly.

After this patch, a module variable could be evaluated in the GDB as p helper::gli where helper is name of the module and gli is the name of the variable. A future patch will add the import module functionality which will remove the need to prefix the name with helper::.

The line number where is module is declared is a best guess at the moment as this information is not part of the GlobalOp.


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

5 Files Affected:

  • (modified) flang/lib/Optimizer/CodeGen/CodeGen.cpp (+16-1)
  • (modified) flang/lib/Optimizer/Transforms/AddDebugInfo.cpp (+91-5)
  • (added) flang/test/Transforms/debug-module-1.f90 (+39)
  • (added) flang/test/Transforms/debug-module-2.f90 (+37)
  • (modified) mlir/lib/Target/LLVMIR/ModuleTranslation.cpp (+7-2)
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index b4705aa479925..4618643f22479 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -2694,6 +2694,18 @@ struct GlobalOpConversion : public fir::FIROpConversion<fir::GlobalOp> {
   mlir::LogicalResult
   matchAndRewrite(fir::GlobalOp global, OpAdaptor adaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
+
+    mlir::LLVM::DIGlobalVariableExpressionAttr dbgExpr;
+
+    if (auto fusedLoc = mlir::dyn_cast<mlir::FusedLoc>(global.getLoc())) {
+      if (auto gvAttr =
+              mlir::dyn_cast_or_null<mlir::LLVM::DIGlobalVariableAttr>(
+                  fusedLoc.getMetadata())) {
+        dbgExpr = mlir::LLVM::DIGlobalVariableExpressionAttr::get(
+            global.getContext(), gvAttr, mlir::LLVM::DIExpressionAttr());
+      }
+    }
+
     auto tyAttr = convertType(global.getType());
     if (auto boxType = mlir::dyn_cast<fir::BaseBoxType>(global.getType()))
       tyAttr = this->lowerTy().convertBoxTypeAsStruct(boxType);
@@ -2702,8 +2714,11 @@ struct GlobalOpConversion : public fir::FIROpConversion<fir::GlobalOp> {
     assert(attributeTypeIsCompatible(global.getContext(), initAttr, tyAttr));
     auto linkage = convertLinkage(global.getLinkName());
     auto isConst = global.getConstant().has_value();
+    mlir::SymbolRefAttr comdat;
+    llvm::ArrayRef<mlir::NamedAttribute> attrs;
     auto g = rewriter.create<mlir::LLVM::GlobalOp>(
-        loc, tyAttr, isConst, linkage, global.getSymName(), initAttr);
+        loc, tyAttr, isConst, linkage, global.getSymName(), initAttr, 0, 0,
+        false, false, comdat, attrs, dbgExpr);
 
     auto module = global->getParentOfType<mlir::ModuleOp>();
     // Add comdat if necessary
diff --git a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
index 908c8fc96f633..0a1acb8b39372 100644
--- a/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
+++ b/flang/lib/Optimizer/Transforms/AddDebugInfo.cpp
@@ -34,6 +34,7 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
+#include <map>
 
 namespace fir {
 #define GEN_PASS_DEF_ADDDEBUGINFO
@@ -48,10 +49,87 @@ class AddDebugInfoPass : public fir::impl::AddDebugInfoBase<AddDebugInfoPass> {
 public:
   AddDebugInfoPass(fir::AddDebugInfoOptions options) : Base(options) {}
   void runOnOperation() override;
+
+private:
+  std::map<std::string, mlir::LLVM::DIModuleAttr> moduleMap;
+
+  mlir::LLVM::DIModuleAttr getOrCreateModuleAttr(
+      const std::string &name, mlir::LLVM::DIFileAttr fileAttr,
+      mlir::LLVM::DIScopeAttr scope, unsigned line, bool decl);
+
+  void handleGlobalOp(fir::GlobalOp glocalOp, mlir::LLVM::DIFileAttr fileAttr,
+                      mlir::LLVM::DIScopeAttr scope);
 };
 
+static uint32_t getLineFromLoc(mlir::Location loc) {
+  uint32_t line = 1;
+  if (auto fileLoc = mlir::dyn_cast<mlir::FileLineColLoc>(loc))
+    line = fileLoc.getLine();
+  return line;
+}
+
 } // namespace
 
+// 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.
+mlir::LLVM::DIModuleAttr AddDebugInfoPass::getOrCreateModuleAttr(
+    const std::string &name, mlir::LLVM::DIFileAttr fileAttr,
+    mlir::LLVM::DIScopeAttr scope, unsigned line, bool decl) {
+  mlir::MLIRContext *context = &getContext();
+  mlir::LLVM::DIModuleAttr modAttr;
+  if (auto iter{moduleMap.find(name)}; iter != moduleMap.end())
+    modAttr = iter->second;
+  else {
+    modAttr = mlir::LLVM::DIModuleAttr::get(
+        context, fileAttr, scope, mlir::StringAttr::get(context, name),
+        mlir::StringAttr(), mlir::StringAttr(), mlir::StringAttr(), line, decl);
+    moduleMap[name] = modAttr;
+  }
+  return modAttr;
+}
+
+void AddDebugInfoPass::handleGlobalOp(fir::GlobalOp globalOp,
+                                      mlir::LLVM::DIFileAttr fileAttr,
+                                      mlir::LLVM::DIScopeAttr scope) {
+  mlir::ModuleOp module = getOperation();
+  mlir::MLIRContext *context = &getContext();
+  fir::DebugTypeGenerator typeGen(module);
+  mlir::OpBuilder builder(context);
+
+  auto result = fir::NameUniquer::deconstruct(globalOp.getSymName());
+  if (result.first != fir::NameUniquer::NameKind::VARIABLE)
+    return;
+
+  unsigned line = getLineFromLoc(globalOp.getLoc());
+
+  // DWARF5 says following about the fortran modules:
+  // A Fortran 90 module may also be represented by a module entry
+  // (but no declaration attribute is warranted because Fortran has no concept
+  // of a corresponding module body).
+  // But in practice, compilers use declaration attribute with a module in cases
+  // where module was defined in another source file (only being used in this
+  // one). The hasInitializationBody() seems to provide the right information
+  // but inverted. It is true where module is actually defined but false where
+  // it is used.
+  // FIXME: Currently we don't have the line number on which a module was
+  // declared. We are using a best guess of line - 1 where line is the source
+  // line of the first member of the module that we encounter.
+
+  if (!result.second.modules.empty())
+    scope = getOrCreateModuleAttr(result.second.modules[0], fileAttr, scope,
+                                  line - 1, !globalOp.hasInitializationBody());
+
+  auto diType = typeGen.convertType(globalOp.getType(), fileAttr, scope,
+                                    globalOp.getLoc());
+  auto gvAttr = mlir::LLVM::DIGlobalVariableAttr::get(
+      context, scope, mlir::StringAttr::get(context, result.second.name),
+      mlir::StringAttr::get(context, globalOp.getName()), fileAttr, line,
+      diType, /*isLocalToUnit*/ false,
+      /*isDefinition*/ globalOp.hasInitializationBody(), /* alignInBits*/ 0);
+  globalOp->setLoc(builder.getFusedLoc({globalOp->getLoc()}, gvAttr));
+}
+
 void AddDebugInfoPass::runOnOperation() {
   mlir::ModuleOp module = getOperation();
   mlir::MLIRContext *context = &getContext();
@@ -91,6 +169,10 @@ void AddDebugInfoPass::runOnOperation() {
       llvm::dwarf::getLanguage("DW_LANG_Fortran95"), fileAttr, producer,
       isOptimized, debugLevel);
 
+  module.walk([&](fir::GlobalOp globalOp) {
+    handleGlobalOp(globalOp, fileAttr, cuAttr);
+  });
+
   module.walk([&](mlir::func::FuncOp funcOp) {
     mlir::Location l = funcOp->getLoc();
     // If fused location has already been created then nothing to do
@@ -131,8 +213,12 @@ void AddDebugInfoPass::runOnOperation() {
     mlir::LLVM::DIFileAttr funcFileAttr =
         mlir::LLVM::DIFileAttr::get(context, fileName, filePath);
 
+    unsigned line = 1;
+    if (auto funcLoc = mlir::dyn_cast<mlir::FileLineColLoc>(l))
+      line = funcLoc.getLine();
     // Only definitions need a distinct identifier and a compilation unit.
     mlir::DistinctAttr id;
+    mlir::LLVM::DIScopeAttr Scope = fileAttr;
     mlir::LLVM::DICompileUnitAttr compilationUnit;
     mlir::LLVM::DISubprogramFlags subprogramFlags =
         mlir::LLVM::DISubprogramFlags{};
@@ -144,13 +230,13 @@ void AddDebugInfoPass::runOnOperation() {
       subprogramFlags =
           subprogramFlags | mlir::LLVM::DISubprogramFlags::Definition;
     }
-    unsigned line = 1;
-    if (auto funcLoc = mlir::dyn_cast<mlir::FileLineColLoc>(l))
-      line = funcLoc.getLine();
+    if (!result.second.modules.empty())
+      Scope = getOrCreateModuleAttr(result.second.modules[0], fileAttr, cuAttr,
+                                    line - 1, false);
 
     auto spAttr = mlir::LLVM::DISubprogramAttr::get(
-        context, id, compilationUnit, fileAttr, funcName, fullName,
-        funcFileAttr, line, line, subprogramFlags, subTypeAttr);
+        context, id, compilationUnit, Scope, funcName, fullName, funcFileAttr,
+        line, line, subprogramFlags, subTypeAttr);
     funcOp->setLoc(builder.getFusedLoc({funcOp->getLoc()}, spAttr));
   });
 }
diff --git a/flang/test/Transforms/debug-module-1.f90 b/flang/test/Transforms/debug-module-1.f90
new file mode 100644
index 0000000000000..2321c7093f422
--- /dev/null
+++ b/flang/test/Transforms/debug-module-1.f90
@@ -0,0 +1,39 @@
+! RUN: %flang_fc1 -emit-fir -debug-info-kind=standalone -mmlir --mlir-print-debuginfo %s -o - | \
+! RUN: fir-opt --cg-rewrite --mlir-print-debuginfo | fir-opt --add-debug-info --mlir-print-debuginfo | FileCheck %s
+
+! CHECK-DAG: #[[I4:.*]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer", sizeInBits = 32, encoding = DW_ATE_signed>
+! CHECK-DAG: #[[R4:.*]] = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "real", sizeInBits = 32, encoding = DW_ATE_float>
+! CHECK-DAG: #[[FILE:.*]] = #llvm.di_file<"debug-module-1.f90" {{.*}}>
+! CHECK-DAG: #[[CU:.*]] = #llvm.di_compile_unit<{{.*}}, file = #[[FILE]], {{.*}}>
+! CHECK-DAG: #[[MOD:.*]] = #llvm.di_module<file = #[[FILE]], scope = #[[CU]], name = "helper", {{.*}}>
+module helper
+! CHECK-DAG: #[[LOC1:.*]] = loc("{{.*}}debug-module-1.f90":[[@LINE+2]]{{.*}})
+! CHECK-DAG: #[[GLR:.*]] = #llvm.di_global_variable<scope = #[[MOD]], name = "glr", linkageName = "_QMhelperEglr", file = #[[FILE]], line = [[@LINE+1]], type = #[[R4]], isDefined = true>
+  real glr
+! CHECK-DAG: #[[LOC2:.*]] = loc("{{.*}}debug-module-1.f90":[[@LINE+2]]{{.*}})
+! CHECK-DAG: #[[GLI:.*]] = #llvm.di_global_variable<scope = #[[MOD]], name = "gli", linkageName = "_QMhelperEgli", file = #[[FILE]], line = [[@LINE+1]], type = #[[I4]], isDefined = true>
+  integer gli
+
+  contains
+! CHECK-DAG: #[[LOC3:.*]] = loc("{{.*}}debug-module-1.f90":[[@LINE+2]]{{.*}})
+! CHECK-DAG: #[[TEST:.*]] = #llvm.di_subprogram<{{.*}}compileUnit = #[[CU]], scope = #[[MOD]], name = "test", linkageName = "_QMhelperPtest", file = #[[FILE]], line = [[@LINE+1]], scopeLine = [[@LINE+1]]{{.*}}>
+    subroutine test()
+    glr = 12.34
+    gli = 67
+
+    end subroutine
+end module helper
+
+program test
+use helper
+implicit none
+
+  glr = 3.14
+  gli = 2
+  call test()
+
+end program test
+
+! CHECK-DAG: loc(fused<#[[GLR]]>[#[[LOC1]]])
+! CHECK-DAG: loc(fused<#[[GLI]]>[#[[LOC2]]])
+! CHECK-DAG: loc(fused<#[[TEST]]>[#[[LOC3]]])
\ No newline at end of file
diff --git a/flang/test/Transforms/debug-module-2.f90 b/flang/test/Transforms/debug-module-2.f90
new file mode 100644
index 0000000000000..9f6bba3ef0470
--- /dev/null
+++ b/flang/test/Transforms/debug-module-2.f90
@@ -0,0 +1,37 @@
+! RUN: %flang_fc1 -emit-llvm -debug-info-kind=standalone %s -o - | FileCheck %s
+
+! CHECK-DAG: ![[FILE:.*]] = !DIFile(filename: {{.*}}debug-module-2.f90{{.*}})
+! CHECK-DAG: ![[FILE2:.*]] = !DIFile(filename: {{.*}}debug-module-2.f90{{.*}})
+! CHECK-DAG: ![[CU:.*]] = distinct !DICompileUnit({{.*}}file: ![[FILE]]{{.*}} globals: ![[GLOBALS:.*]])
+! CHECK-DAG: ![[MOD:.*]] = !DIModule(scope: ![[CU]], name: "helper", file: ![[FILE]]{{.*}})
+! CHECK-DAG: ![[R4:.*]] = !DIBasicType(name: "real", size: 32, encoding: DW_ATE_float)
+! CHECK-DAG: ![[I4:.*]] = !DIBasicType(name: "integer", size: 32, encoding: DW_ATE_signed)
+module helper
+! CHECK-DAG: ![[GLR:.*]] = distinct !DIGlobalVariable(name: "glr", linkageName: "_QMhelperEglr", scope: ![[MOD]], file: ![[FILE]], line: [[@LINE+2]], type: ![[R4]], isLocal: false, isDefinition: true)
+! CHECK-DAG: ![[GLRX:.*]] = !DIGlobalVariableExpression(var: ![[GLR]], expr: !DIExpression())
+  real glr
+
+! CHECK-DAG: ![[GLI:.*]] = distinct !DIGlobalVariable(name: "gli", linkageName: "_QMhelperEgli", scope: ![[MOD]], file: ![[FILE]], line: [[@LINE+2]], type: ![[I4]], isLocal: false, isDefinition: true)
+! CHECK-DAG: ![[GLIX:.*]] = !DIGlobalVariableExpression(var: ![[GLI]], expr: !DIExpression())
+  integer gli
+
+  contains
+!CHECK-DAG: !DISubprogram(name: "test", linkageName: "_QMhelperPtest", scope: ![[MOD]], file: ![[FILE2]], line: [[@LINE+1]]{{.*}}unit: ![[CU]])
+    subroutine test()
+    glr = 12.34
+    gli = 67
+
+    end subroutine
+end module helper
+
+program test
+use helper
+implicit none
+
+  glr = 3.14
+  gli = 2
+  call test()
+
+end program test
+
+! CHECK-DAG: ![[GLOBALS]] = !{![[GLIX]], ![[GLRX]]}
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index 669b95a9c6a5b..4f4a3ea046241 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -1030,10 +1030,15 @@ LogicalResult ModuleTranslation::convertGlobals() {
       llvm::DIGlobalVariable *diGlobalVar = diGlobalExpr->getVariable();
       var->addDebugInfo(diGlobalExpr);
 
+      // For fortran, the scope hierarchy can be
+      // variable -> module -> compile unit
+      llvm::DIScope *scope = diGlobalVar->getScope();
+      if (llvm::DIModule *mod = dyn_cast_if_present<llvm::DIModule>(scope))
+        scope = mod->getScope();
+
       // Get the compile unit (scope) of the the global variable.
       if (llvm::DICompileUnit *compileUnit =
-              dyn_cast_if_present<llvm::DICompileUnit>(
-                  diGlobalVar->getScope())) {
+              dyn_cast_if_present<llvm::DICompileUnit>(scope)) {
         // Update the compile unit with this incoming global variable expression
         // during the finalizing step later.
         allGVars[compileUnit].push_back(diGlobalExpr);

Comment on lines 1033 to 1044
// For fortran, the scope hierarchy can be
// variable -> module -> compile unit
llvm::DIScope *scope = diGlobalVar->getScope();
if (llvm::DIModule *mod = dyn_cast_if_present<llvm::DIModule>(scope))
scope = mod->getScope();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to MLIR should go in as a separate patch. And a test is required for the MLIR changes in I guess mlir/test/Target/LLVMIR/llvmir-debug.mlir.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting me know. I have now opened #91604 with this change and a testcase. I will remove this bit from this PR.

Comment on lines +73 to +114
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this mean that if a module variable or subroutine is not used we will not have the information that the module is used via a USE statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In debugger, one should be able to evaluate module variables using module::var syntax even in functions where the module is not used (using use keyword). For functions where it is used, evaluation without module:: should work too (once we have implemented the imported module functionality).

I also tried multi file examples where module is defined in one file and used in others. In such cases, compilers (e.g. gfortran, classic flang) generate a module entry in both the compile units. To differentiate, they set decl attribute to true in files where it is just used. I have tried to follow the similar approach. I found that GlobalOp::hasInitializationBody() gave me this information. It was true in compile unit where the module was actually defined and false where it was just used.

abidh added a commit to abidh/llvm-project that referenced this pull request May 9, 2024
Currently, only those global variables which are at compile unit scope
are added to the 'Globals' list of the compile unit. This does not work
for module variables of fortran where hierarchy can be
variable -> module -> compile unit.

To fix this, if a variable scope points to a module, we walk one level
up and see if module is in the compile unit scope.

This was initially part of llvm#91582 which adds debug information for
fortran module variables. Kiran pointed out that MLIR changes should
go in separate PRs.
abidh added a commit that referenced this pull request May 14, 2024
Currently, only those global variables which are at compile unit scope
are added to the 'globals' list of the DICompileUnit. This does not work
for languages which support modules (e.g. Fortran) where hierarchy
can be
variable -> module -> compile unit.

To fix this, if a variable scope points to a module, we walk one level
up and see if module is in the compile unit scope.

This was initially part of #91582 which adds debug information for
Fortran module variables. @kiranchandramohan pointed out that MLIR
changes should go in separate PRs.
Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks!

@@ -34,6 +34,7 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/raw_ostream.h"
#include <map>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is no longer needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. Removed.

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch adds debug info for other kind of globals (besides module variables) like for eg. x in the following case. Is that the intention? If so a test would be good.

subroutine abc
  integer :: x = 2
  print *, x
end subroutine

Commonblocks also are globals but I did not see debug for them.

I am guessing the following points will come in future patches.
-> Renamed module variables using use statement.
-> Private module variables. Can they be distinguished in DWARF?
-> Submodules


! CHECK-DAG: loc(fused<#[[GLR]]>[#[[LOC1]]])
! CHECK-DAG: loc(fused<#[[GLI]]>[#[[LOC2]]])
! CHECK-DAG: loc(fused<#[[TEST]]>[#[[LOC3]]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: end of line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@kiranchandramohan
Copy link
Contributor

There is a plan for a pass (#73829) that would convert some constants to globals. Would it be OK to mark this as a Global Variable?

@abidh
Copy link
Contributor Author

abidh commented May 15, 2024

The patch adds debug info for other kind of globals (besides module variables) like for eg. x in the following case. Is that the intention? If so a test would be good.

subroutine abc
  integer :: x = 2
  print *, x
end subroutine

This was not intentional. The variable with save attribute should be added as global but with scope limited to the subroutine. I will add a check to skip them in this patch.

On a related note, while processing DeclareOp of such variables, are you aware of anything that will tell me that these variables have save attribute? I ask because I noticed that they are being added as local variable too.

Commonblocks also are globals but I did not see debug for them.

Yes, common blocks are not supported yet.

I am guessing the following points will come in future patches. -> Renamed module variables using use statement. -> Private module variables. Can they be distinguished in DWARF? -> Submodules

Private module variable should work with this patch. I was not aware of the renaming by the use statement syntax. I will add it in my todo list. DWARF has concept of DW_TAG_imported_declaration for such cases. I will have to see if we have enough information to generate it.

@abidh
Copy link
Contributor Author

abidh commented May 15, 2024

There is a plan for a pass (#73829) that would convert some constants to globals. Would it be OK to mark this as a Global Variable?

Currently, we add variable only if deconstructed name kind is fir::NameUniquer::NameKind::VARIABLE. I think that should filter out the generated globals.

return;

scope = getOrCreateModuleAttr(result.second.modules[0], fileAttr, scope,
line - 1, !globalOp.hasInitializationBody());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use globalOp.isInitialized() here and below. In some cases, the initial value is defined as an attribute, not with operations inside the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you suggestions. Changed now.

@kiranchandramohan
Copy link
Contributor

On a related note, while processing DeclareOp of such variables, are you aware of anything that will tell me that these variables have save attribute? I ask because I noticed that they are being added as local variable too.

I am not aware.

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG.

fir::DebugTypeGenerator typeGen(module);
mlir::OpBuilder builder(context);

auto result = fir::NameUniquer::deconstruct(globalOp.getSymName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could you spell auto?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Although I noticed that auto has been used in most places with fir::NameUniquer::deconstruct.

Comment on lines 120 to 121
if (auto iter{moduleMap.find(name)}; iter != moduleMap.end())
modAttr = iter->getValue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Braces here to match the else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

else {
modAttr = mlir::LLVM::DIModuleAttr::get(
context, fileAttr, scope, mlir::StringAttr::get(context, name),
mlir::StringAttr(), mlir::StringAttr(), mlir::StringAttr(), line, decl);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: comments here to spell the default attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

scope = getOrCreateModuleAttr(result.second.modules[0], fileAttr, scope,
line - 1, !globalOp.isInitialized());

auto diType = typeGen.convertType(globalOp.getType(), fileAttr, scope,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Spell the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 1 to 2
! RUN: %flang_fc1 -emit-llvm -debug-info-kind=standalone %s -o - | FileCheck %s
! RUN: %flang_fc1 -emit-llvm -debug-info-kind=line-tables-only %s -o - | FileCheck --check-prefix=LINEONLY %s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM IR tests should go to the Integration directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the testcase to Integration folder. There may be other IR testcase that I added and I will move them in separate PR.

Comment on lines 1 to 2
! RUN: %flang_fc1 -emit-fir -debug-info-kind=standalone -mmlir --mlir-print-debuginfo %s -o - | \
! RUN: fir-opt --cg-rewrite --mlir-print-debuginfo | fir-opt --add-debug-info --mlir-print-debuginfo | FileCheck %s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to test the pass only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AddDebugInfo pass now operates on XDeclareOp so we need to run CodeGenRewrite before to convert the DeclareOp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to convey the following:
-> If it is the AddDebugInfo pass that you want to test, then give it the input after cg-rewrite and test its output.
-> If it is the CodeGen changes that you want to test, then give it the input before Codegen and test its output.

I am guessing the LLVM IR changes helps it easier to review and hence it might be OK to add it to the Integration directory.

Generally tests that specifically checks the changes are easier to review and also maintain. This way of writing helps to ensure that when some unrelated changes happen these tests are not affected. For testing end to end flows we should ideally set up a separate buildbot or move these to the llvm-testsuite/Fortran/Debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the suggestion on using output of cg-rewrite to test the AddDebugInfo. My initial motivation for basing test on fortran source code was that it was more readable.

As action items,

I will update AddDebugInfo tests to be based on the output of cg-rewrite
I will add codegen tests when it is involved.
I will keep adding llvm ir tests in Integration for now.

Copy link

github-actions bot commented May 20, 2024

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

abidh added 8 commits May 20, 2024 19:29
This PR add supports for module variables and function. The module
variables are added as global variables but their scope is set to
module instead of compile unit. The scope of function declared inside
a module is also set accordingly.

After this patch, a module variable could be evaluated in the GDB as
`p helper::gli` where helper is name of the module and gli is the name
of the variable. A future patch will add the import module functionality
which will remove the need to prefix the name with helper::.

The line number where is module is declared is a best guess at the
moment as this information is not part of the GlobalOp.
Following changes were done.

1. Remove MLIR changes as they have been moved to separate PR.

2. Replace std::map with llvm::StringMap

3. Use a direct for loop instead of module.walk.
1. Skip variables if module list is empty.

2. Add a missing line at the end of a file.
This address one of the review comments.
Fixed nits and move IR tests to Integration folder.
@Dinistro Dinistro removed their request for review May 21, 2024 08:26
Add test files that work on fir and test individual passes. Remove the
fortran testcase.
@abidh abidh merged commit f156b9c into llvm:main May 22, 2024
4 checks passed
@abidh abidh deleted the modules1 branch May 22, 2024 21:00
abidh added a commit that referenced this pull request Jun 21, 2024
@kiranchandramohan mentioned
[here](#91582 (comment))
that LLVM IR tests should go in the Integration folder. He also
mentioned
[here](#91582 (comment))
that tests for `add-debug-info` pass should test that pass only. There
were some tests which were added before his comments so I have cleaned
them in this PR. The following changes were made.

1. Move LLVM IR tests to `Integration` folder.
2. Change tests from f90 to fir and only test changes done by
`add-debug-info` pass.
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
@kiranchandramohan mentioned
[here](llvm#91582 (comment))
that LLVM IR tests should go in the Integration folder. He also
mentioned
[here](llvm#91582 (comment))
that tests for `add-debug-info` pass should test that pass only. There
were some tests which were added before his comments so I have cleaned
them in this PR. The following changes were made.

1. Move LLVM IR tests to `Integration` folder.
2. Change tests from f90 to fir and only test changes done by
`add-debug-info` pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:codegen flang:fir-hlfir flang Flang issues not falling into any other category mlir:llvm mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants