-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Conversation
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-flang-fir-hlfir Author: Abid Qadeer (abidh) ChangesThis 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 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:
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);
|
// 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(); | ||
|
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this 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> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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]]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: end of line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
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? |
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
Yes, common blocks are not supported yet.
Private module variable should work with this patch. I was not aware of the renaming by the |
Currently, we add variable only if deconstructed name kind is |
return; | ||
|
||
scope = getOrCreateModuleAttr(result.second.modules[0], fileAttr, scope, | ||
line - 1, !globalOp.hasInitializationBody()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I am not aware. |
There was a problem hiding this 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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
if (auto iter{moduleMap.find(name)}; iter != moduleMap.end()) | ||
modAttr = iter->getValue(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Spell the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
! 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
! 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
Add test files that work on fir and test individual passes. Remove the fortran testcase.
@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.
@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.
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.