[mlir][flang] Improve handling of fortran module variables. #91604

abidh · 2024-05-09T15:08:31Z

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 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 #91582 which adds debug information for Fortran module variables. @kiranchandramohan 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 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.

llvmbot · 2024-05-09T15:09:04Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-llvm

Author: Abid Qadeer (abidh)

Changes

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 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 #91582 which adds debug information for Fortran module variables. @kiranchandramohan pointed out that MLIR changes should go in separate PRs.

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

2 Files Affected:

(modified) mlir/lib/Target/LLVMIR/ModuleTranslation.cpp (+8-2)
(modified) mlir/test/Target/LLVMIR/llvmir-debug.mlir (+23)

diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index 669b95a9c6a5b..717a32e4f980f 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -1029,11 +1029,17 @@ LogicalResult ModuleTranslation::convertGlobals() {
           debugTranslation->translateGlobalVariableExpression(op.getDbgExpr());
       llvm::DIGlobalVariable *diGlobalVar = diGlobalExpr->getVariable();
       var->addDebugInfo(diGlobalExpr);
+      // For fortran, the scope hierarchy can be
+      // variable -> module -> compile unit
+      // If a variable scope points to Module then we get its parent scope so
+      // that globals get added to the 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);
diff --git a/mlir/test/Target/LLVMIR/llvmir-debug.mlir b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
index 1f0fc969364ac..b561ac5968ccc 100644
--- a/mlir/test/Target/LLVMIR/llvmir-debug.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
@@ -311,6 +311,29 @@ llvm.mlir.global external @global_with_expr_2() {addr_space = 0 : i32, dbg_expr
 
 // -----
 
+// CHECK: @module_global_1 = external global i64, !dbg {{.*}}
+// CHECK: @module_global_2 = external global i64, !dbg {{.*}}
+// CHECK: !llvm.module.flags = !{{{.*}}}
+// CHECK: !llvm.dbg.cu = !{{{.*}}}
+// CHECK-DAG: ![[FILE:.*]] = !DIFile(filename: "test.f90", directory: "existence")
+// CHECK-DAG: ![[TYPE:.*]] = !DIBasicType(name: "integer", size: 64, encoding: DW_ATE_signed)
+// CHECK-DAG: ![[SCOPE:.*]] = distinct !DICompileUnit(language: DW_LANG_Fortran95, file: ![[FILE]], producer: "MLIR", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: ![[GVALS:.*]])
+// CHECK-DAG: ![[SCOPE1:.*]] = !DIModule(scope: ![[SCOPE]], name: "module2", file: ![[FILE]], line: 120)
+// CHECK-DAG: ![[GVAR0:.*]] = distinct !DIGlobalVariable(name: "module_global_1", linkageName: "module_global_1", scope: ![[SCOPE1]], file: ![[FILE]], line: 121, type: ![[TYPE]], isLocal: false, isDefinition: true)
+// CHECK-DAG: ![[GVAR1:.*]] = distinct !DIGlobalVariable(name: "module_global_2", linkageName: "module_global_2", scope: ![[SCOPE1]], file: ![[FILE]], line: 122, type: ![[TYPE]], isLocal: false, isDefinition: true)
+// CHECK-DAG: ![[GEXPR0:.*]] = !DIGlobalVariableExpression(var: ![[GVAR0]], expr: !DIExpression())
+// CHECK-DAG: ![[GEXPR1:.*]] = !DIGlobalVariableExpression(var: ![[GVAR1]], expr: !DIExpression())
+// CHECK-DAG: ![[GVALS]] = !{![[GEXPR0]], ![[GEXPR1]]}
+
+#di_file = #llvm.di_file<"test.f90" in "existence">
+#di_compile_unit = #llvm.di_compile_unit<id = distinct[0]<>, sourceLanguage = DW_LANG_Fortran95, file = #di_file, producer = "MLIR", isOptimized = true, emissionKind = Full>
+#di_basic_type = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer", sizeInBits = 64, encoding = DW_ATE_signed>
+#di_module = #llvm.di_module<file = #di_file, scope = #di_compile_unit, name = "module2", configMacros = "", includePath = "", apinotes = "", line = 120, isDecl = false >
+llvm.mlir.global external @module_global_1() {dbg_expr = #llvm.di_global_variable_expression<var = <scope = #di_module, name = "module_global_1", linkageName = "module_global_1", file = #di_file, line = 121, type = #di_basic_type, isLocalToUnit = false, isDefined = true>, expr = <>>} : i64
+llvm.mlir.global external @module_global_2() {dbg_expr = #llvm.di_global_variable_expression<var = <scope = #di_module, name = "module_global_2", linkageName = "module_global_2", file = #di_file, line = 122, type = #di_basic_type, isLocalToUnit = false, isDefined = true>, expr = <>>} : i64
+
+// -----
+
 // Nameless and scopeless global constant.
 
 // CHECK-LABEL: @.str.1 = external constant [10 x i8]

Dinistro · 2024-05-09T15:41:11Z

Can you elaborate on why the variable's scope is not already set accordingly by flang? I assume that there is some reason for this, but I'm not familiar enough with flang to know this detail.

abidh · 2024-05-09T17:10:48Z

Can you elaborate on why the variable's scope is not already set accordingly by flang? I assume that there is some reason for this, but I'm not familiar enough with flang to know this detail.

Global variable in the context of DWARF is a bit of a loaded term. Same constructs that are used for C/C++ global variables are also used for module variables in Fortran (and namespace variables in C++). MLIR code is assuming that global variable will always have compile unit scope which may not be correct in those cases.

Take the example of namespace in c++ below. The debug metadata was generated by clang. You can see that scope of the global variable a is set to namespace (and not to compile unit) but it still gets added to globals in DICompileUnit.

namespace test
{
    int a;
}

!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
!1 = distinct !DIGlobalVariable(name: "a", linkageName: "_ZN4test1aE", scope: !2 ...)
!2 = !DINamespace(name: "test", scope: null)
!5 = distinct !DICompileUnit(... globals: !6 ...)
!6 = !{!0}

Fortran module variables are also represented as global variable in DWARF. But to recognize that they are part of a module, their scope is set to module and not to compile unit. In summary, the scope of the variable is correct. The MLIR check is overly restrictive and this PR amends it for this specific case.

kiranchandramohan

Thanks @abidh for separating out this patch.

Take the example of namespace in c++ below. The debug metadata was generated by clang. You can see that scope of the global variable a is set to namespace (and not to compile unit) but it still gets added to globals in DICompileUnit.

Is it possible to have common handling for Namespaces and Modules or will Namespace be another if case?

kiranchandramohan · 2024-05-09T21:40:01Z

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

+      // For fortran, the scope hierarchy can be
+      // variable -> module -> compile unit


Modules are there in C++ also these days. So can we say "In languages like C++, Fortran etc variables ..."

Or alternatively,
-> remove the mention of Fortran. Just say that this hierarchy is allowed for languages supporting modules.
-> For Modula 2, Fortran like languages

Thanks for the suggestion. I have updated the comments.

abidh · 2024-05-10T09:59:13Z

Thanks @abidh for separating out this patch.

Take the example of namespace in c++ below. The debug metadata was generated by clang. You can see that scope of the global variable a is set to namespace (and not to compile unit) but it still gets added to globals in DICompileUnit.

Is it possible to have common handling for Namespaces and Modules or will Namespace be another if case?

The namespaces are a bit different as they can be fragmented in multiple files. So their handling may indeed be slightly different if ever a C++ frontend targets mlir.

kiranchandramohan

LGTM.

kiranchandramohan · 2024-05-10T10:23:02Z

Thanks @abidh for separating out this patch.

Take the example of namespace in c++ below. The debug metadata was generated by clang. You can see that scope of the global variable a is set to namespace (and not to compile unit) but it still gets added to globals in DICompileUnit.

Is it possible to have common handling for Namespaces and Modules or will Namespace be another if case?

The namespaces are a bit different as they can be fragmented in multiple files. So their handling may indeed be slightly different if ever a C++ frontend targets mlir.

The clangir project (https://github.com/llvm/clangir) is being upstreamed. https://discourse.llvm.org/t/rfc-upstreaming-clangir/76587

Dinistro · 2024-05-10T11:13:13Z

Fortran module variables are also represented as global variable in DWARF. But to recognize that they are part of a module, their scope is set to module and not to compile unit. In summary, the scope of the variable is correct. The MLIR check is overly restrictive and this PR amends it for this specific case.

Thanks for the clarification. Ideally, we would model DICompile units to also have a globals field, but this would result in circular attribute structures. While supporting such constructs is possible, it's quite involved to get this to work and is tricky to work with.

Dinistro

LGTM % nit

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

abidh · 2024-05-10T15:27:59Z

Fortran module variables are also represented as global variable in DWARF. But to recognize that they are part of a module, their scope is set to module and not to compile unit. In summary, the scope of the variable is correct. The MLIR check is overly restrictive and this PR amends it for this specific case.

Thanks for the clarification. Ideally, we would model DICompile units to also have a globals field, but this would result in circular attribute structures. While supporting such constructs is possible, it's quite involved to get this to work and is tricky to work with.

DICompileUnit does have a globals field. Although it is not exposed through DIBuilder::createCompileUnit. Generally, when DICompileUnit is created, you don't have the list of globals. The DICompileUnit has replaceGlobalVariables to set the list of globals once they have been processed, providing a way to break circular dependency.

Dinistro · 2024-05-10T15:37:28Z

Fortran module variables are also represented as global variable in DWARF. But to recognize that they are part of a module, their scope is set to module and not to compile unit. In summary, the scope of the variable is correct. The MLIR check is overly restrictive and this PR amends it for this specific case.

Thanks for the clarification. Ideally, we would model DICompile units to also have a globals field, but this would result in circular attribute structures. While supporting such constructs is possible, it's quite involved to get this to work and is tricky to work with.

DICompileUnit does have a globals field. Although it is not exposed through DIBuilder::createCompileUnit. Generally, when DICompileUnit is created, you don't have the list of globals. The DICompileUnit has replaceGlobalVariables to set the list of globals once they have been processed, providing a way to break circular dependency.

I was referring to the MLIR attribute that is supposed to be isomorphic to the LLVM representation. Modeling the equivalent in MLIR's LLVM dialect would be much more complicate due to limits of the attribute subsystem.
Either way, if this is how it's supposed to be modelled, I'm fine with this change 😄

Dinistro · 2024-05-13T10:39:13Z

mlir/test/Target/LLVMIR/llvmir-debug.mlir

+#di_basic_type = #llvm.di_basic_type<tag = DW_TAG_base_type, name = "integer", sizeInBits = 64, encoding = DW_ATE_signed>
+#di_module = #llvm.di_module<file = #di_file, scope = #di_compile_unit, name = "module2", configMacros = "", includePath = "", apinotes = "", line = 120, isDecl = false >
+llvm.mlir.global external @module_global_1() {dbg_expr = #llvm.di_global_variable_expression<var = <scope = #di_module, name = "module_global_1", linkageName = "module_global_1", file = #di_file, line = 121, type = #di_basic_type, isLocalToUnit = false, isDefined = true>, expr = <>>} : i64
+llvm.mlir.global external @module_global_2() {dbg_expr = #llvm.di_global_variable_expression<var = <scope = #di_module, name = "module_global_2", linkageName = "module_global_2", file = #di_file, line = 122, type = #di_basic_type, isLocalToUnit = false, isDefined = true>, expr = <>>} : i64


Do we require both of these for the test here. They seem identical, and only using this to ensure we fill a list properly seems a bit overkill, given that this is tested in other places already.

Yes, one would be enough. I would update it.

As was pointed out in the review, one variable is enough for the test.

Dinistro · 2024-05-15T05:33:50Z

Thanks for addressing the comments. Feel free to land, btw :)

abidh requested review from zyx-billy, kiranchandramohan, Dinistro, dpalermo and kiranktp May 9, 2024 15:08

llvmbot added mlir:llvm mlir labels May 9, 2024

abidh mentioned this pull request May 9, 2024

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

Merged

kiranchandramohan reviewed May 9, 2024

View reviewed changes

kiranchandramohan approved these changes May 10, 2024

View reviewed changes

Dinistro approved these changes May 10, 2024

View reviewed changes

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp Show resolved Hide resolved

Update comments as per review suggestions.

fa2105e

Dinistro reviewed May 13, 2024

View reviewed changes

Remove redundant variable from the test.

6cdaca7

As was pointed out in the review, one variable is enough for the test.

abidh merged commit 77b80bd into llvm:main May 14, 2024

abidh deleted the mlir_module branch June 4, 2024 09:43

		// For fortran, the scope hierarchy can be
		// variable -> module -> compile unit

[mlir][flang] Improve handling of fortran module variables. #91604

[mlir][flang] Improve handling of fortran module variables. #91604

Uh oh!

Conversation

abidh commented May 9, 2024

Uh oh!

llvmbot commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dinistro commented May 9, 2024

Uh oh!

abidh commented May 9, 2024

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan May 9, 2024

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan May 10, 2024

Choose a reason for hiding this comment

Uh oh!

abidh May 13, 2024

Choose a reason for hiding this comment

Uh oh!

abidh commented May 10, 2024

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan commented May 10, 2024

Uh oh!

Dinistro commented May 10, 2024

Uh oh!

Dinistro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abidh commented May 10, 2024

Uh oh!

Dinistro commented May 10, 2024

Uh oh!

Dinistro May 13, 2024

Choose a reason for hiding this comment

Uh oh!

abidh May 13, 2024

Choose a reason for hiding this comment

Uh oh!

abidh May 14, 2024

Choose a reason for hiding this comment

Uh oh!

Dinistro commented May 15, 2024

Uh oh!

Uh oh!

llvmbot commented May 9, 2024 •

edited

Loading