[MLIR][LLVM] Fuse Scope into CallsiteLoc Callee #74546
Conversation
|
One next step I'm definitely interested in is to better define the semantics of a fused DI scope in general. I'm tempted to say that we should require DI scopes only be fused to leaf locations (those that don't contain other locations. Like FileLineCol loc), since that would make the most sense for LLVM, and cause the least confusion in semantics. Not sure if I'm missing any use cases that way though, or if there's a good way to enforce that besides when translating. |
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-llvm Author: Billy Zhu (zyx-billy) ChangesThere's an issue in the translator today where, for a CallsiteLoc, if the callee does not have a DI scope (perhaps due to compile options or optimizations), it may get propagated the DI scope of its callsite's parent function, which will create a non-existent DILocation combining line & col number from one file, and the filename from another. The root problem is we cannot propagate the parent scope when translating the callee location, as it no longer applies to inlined locations (see code diff and hopefully this will make sense). To facilitate this, the importer is also changed so that callee scopes are fused with the callee FileLineCol loc, instead of on the Callsite loc itself. This comes with the benefit that we now have a symmetric Callsite loc representation. If we required the callee scope be always annotated on the Callsite loc, it would be hard for generic inlining passes to maintain that, since it would have to somehow understand the semantics of the fused metadata and pull it out while inlining. Full diff: https://github.com/llvm/llvm-project/pull/74546.diff 4 Files Affected:
diff --git a/mlir/lib/Target/LLVMIR/DebugImporter.cpp b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
index 89d34e4e6f6cf..13b81d134cbe4 100644
--- a/mlir/lib/Target/LLVMIR/DebugImporter.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugImporter.cpp
@@ -282,14 +282,15 @@ Location DebugImporter::translateLoc(llvm::DILocation *loc) {
Location result = FileLineColLoc::get(context, loc->getFilename(),
loc->getLine(), loc->getColumn());
- // Add call site information, if available.
- if (llvm::DILocation *inlinedAt = loc->getInlinedAt())
- result = CallSiteLoc::get(result, translateLoc(inlinedAt));
-
// Add scope information.
assert(loc->getScope() && "expected non-null scope");
result = FusedLocWith<DIScopeAttr>::get({result}, translate(loc->getScope()),
context);
+
+ // Add call site information, if available.
+ if (llvm::DILocation *inlinedAt = loc->getInlinedAt())
+ result = CallSiteLoc::get(result, translateLoc(inlinedAt));
+
return result;
}
diff --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index 26950d7287304..f2889e0dc407e 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -330,7 +330,9 @@ llvm::DILocation *DebugTranslation::translateLoc(Location loc,
if (auto callLoc = dyn_cast<CallSiteLoc>(loc)) {
// For callsites, the caller is fed as the inlinedAt for the callee.
auto *callerLoc = translateLoc(callLoc.getCaller(), scope, inlinedAt);
- llvmLoc = translateLoc(callLoc.getCallee(), scope, callerLoc);
+ llvmLoc = translateLoc(callLoc.getCallee(), nullptr, callerLoc);
+ if (!llvmLoc)
+ return callerLoc; // Fallback: Ignore callee if it has no debug scope.
} else if (auto fileLoc = dyn_cast<FileLineColLoc>(loc)) {
// A scope of a DILocation cannot be null.
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info.ll b/mlir/test/Target/LLVMIR/Import/debug-info.ll
index c8f40cfeabdb7..f8bf00bbf3f6d 100644
--- a/mlir/test/Target/LLVMIR/Import/debug-info.ll
+++ b/mlir/test/Target/LLVMIR/Import/debug-info.ll
@@ -33,11 +33,11 @@ define i32 @instruction_loc(i32 %arg1) {
; CHECK-DAG: #[[SP:.+]] = #llvm.di_subprogram<compileUnit = #{{.*}}, scope = #{{.*}}, name = "instruction_loc"
; CHECK-DAG: #[[CALLEE:.+]] = #llvm.di_subprogram<compileUnit = #{{.*}}, scope = #{{.*}}, name = "callee"
; CHECK-DAG: #[[FILE_LOC]] = loc(fused<#[[SP]]>[#[[RAW_FILE_LOC]]])
-; CHECK-DAG: #[[CALLEE_LOC:.+]] = loc("debug-info.ll":7:4)
+; CHECK-DAG: #[[RAW_CALLEE_LOC:.+]] = loc("debug-info.ll":7:4)
+; CHECK-DAG: #[[CALLEE_LOC:.+]] = loc(fused<#[[CALLEE]]>[#[[RAW_CALLEE_LOC]]])
; CHECK-DAG: #[[RAW_CALLER_LOC:.+]] = loc("debug-info.ll":2:2)
; CHECK-DAG: #[[CALLER_LOC:.+]] = loc(fused<#[[SP]]>[#[[RAW_CALLER_LOC]]])
-; CHECK-DAG: #[[RAW_CALLSITE_LOC:.+]] = loc(callsite(#[[CALLEE_LOC]] at #[[CALLER_LOC]]))
-; CHECK-DAG: #[[CALLSITE_LOC]] = loc(fused<#[[CALLEE]]>[#[[RAW_CALLSITE_LOC]]])
+; CHECK-DAG: #[[CALLSITE_LOC:.+]] = loc(callsite(#[[CALLEE_LOC]] at #[[CALLER_LOC]]))
!llvm.dbg.cu = !{!1}
!llvm.module.flags = !{!0}
diff --git a/mlir/test/Target/LLVMIR/llvmir-debug.mlir b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
index 74e657e10c115..fe7f1b96d3239 100644
--- a/mlir/test/Target/LLVMIR/llvmir-debug.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-debug.mlir
@@ -100,20 +100,23 @@ llvm.func @func_with_debug(%arg: i64) {
// CHECK: call void @llvm.dbg.value(metadata i64 %[[ARG]], metadata ![[NO_NAME_VAR:[0-9]+]], metadata !DIExpression())
llvm.intr.dbg.value #noNameVariable = %arg : i64
- // CHECK: call void @func_no_debug(), !dbg ![[CALLSITE_LOC:[0-9]+]]
- llvm.call @func_no_debug() : () -> () loc(callsite("mysource.cc":3:4 at "mysource.cc":5:6))
-
// CHECK: call void @func_no_debug(), !dbg ![[FILE_LOC:[0-9]+]]
llvm.call @func_no_debug() : () -> () loc("foo.mlir":1:2)
// CHECK: call void @func_no_debug(), !dbg ![[NAMED_LOC:[0-9]+]]
llvm.call @func_no_debug() : () -> () loc("named"("foo.mlir":10:10))
+ // CHECK: call void @func_no_debug(), !dbg ![[CALLSITE_LOC:[0-9]+]]
+ llvm.call @func_no_debug() : () -> () loc(callsite("nodebug.cc":3:4 at "mysource.cc":5:6))
+
+ // CHECK: call void @func_no_debug(), !dbg ![[CALLSITE_LOC:[0-9]+]]
+ llvm.call @func_no_debug() : () -> () loc(callsite("nodebug.cc":3:4 at fused<#sp0>["mysource.cc":5:6]))
+
// CHECK: call void @func_no_debug(), !dbg ![[FUSED_LOC:[0-9]+]]
- llvm.call @func_no_debug() : () -> () loc(fused[callsite("mysource.cc":5:6 at "mysource.cc":1:1), "mysource.cc":1:1])
+ llvm.call @func_no_debug() : () -> () loc(fused[callsite(fused<#callee>["mysource.cc":5:6] at "mysource.cc":1:1), "mysource.cc":1:1])
// CHECK: add i64 %[[ARG]], %[[ARG]], !dbg ![[FUSEDWITH_LOC:[0-9]+]]
- %sum = llvm.add %arg, %arg : i64 loc(fused<#callee>[callsite("foo.mlir":2:4 at fused<#sp0>["foo.mlir":28:5])])
+ %sum = llvm.add %arg, %arg : i64 loc(callsite(fused<#callee>["foo.mlir":2:4] at fused<#sp0>["foo.mlir":28:5]))
llvm.return
} loc(fused<#sp0>["foo.mlir":1:1])
@@ -148,7 +151,7 @@ llvm.func @empty_types() {
// CHECK: ![[BLOCK_LOC]] = distinct !DILexicalBlock(scope: ![[FUNC_LOC]])
// CHECK: ![[NO_NAME_VAR]] = !DILocalVariable(scope: ![[BLOCK_LOC]])
-// CHECK-DAG: ![[CALLSITE_LOC]] = !DILocation(line: 3, column: 4,
+// CHECK-DAG: ![[CALLSITE_LOC]] = !DILocation(line: 5, column: 6,
// CHECK-DAG: ![[FILE_LOC]] = !DILocation(line: 1, column: 2,
// CHECK-DAG: ![[NAMED_LOC]] = !DILocation(line: 10, column: 10
// CHECK-DAG: ![[FUSED_LOC]] = !DILocation(line: 1, column: 1
@@ -186,7 +189,7 @@ llvm.func @empty_types() {
#di_label = #llvm.di_label<scope = #di_lexical_block_file, name = "label", file = #di_file, line = 42>
#loc0 = loc("foo.mlir":0:0)
-#loc1 = loc(callsite(#loc0 at fused<#di_subprogram>["foo.mlir":4:2]))
+#loc1 = loc(callsite(fused<#di_lexical_block_file>[#loc0] at fused<#di_subprogram>["foo.mlir":4:2]))
// CHECK-LABEL: define i32 @func_with_inlined_dbg_value(
// CHECK-SAME: i32 %[[ARG:.*]]) !dbg ![[OUTER_FUNC:[0-9]+]]
@@ -194,9 +197,9 @@ llvm.func @func_with_inlined_dbg_value(%arg0: i32) -> (i32) {
// CHECK: call void @llvm.dbg.value(metadata i32 %[[ARG]], metadata ![[VAR_LOC0:[0-9]+]], metadata !DIExpression()), !dbg ![[DBG_LOC0:.*]]
llvm.intr.dbg.value #di_local_variable0 = %arg0 : i32 loc(fused<#di_subprogram>[#loc0])
// CHECK: call void @llvm.dbg.value(metadata i32 %[[ARG]], metadata ![[VAR_LOC1:[0-9]+]], metadata !DIExpression()), !dbg ![[DBG_LOC1:.*]]
- llvm.intr.dbg.value #di_local_variable1 = %arg0 : i32 loc(fused<#di_lexical_block_file>[#loc1])
+ llvm.intr.dbg.value #di_local_variable1 = %arg0 : i32 loc(#loc1)
// CHECK: call void @llvm.dbg.label(metadata ![[LABEL:[0-9]+]]), !dbg ![[DBG_LOC1:.*]]
- llvm.intr.dbg.label #di_label loc(fused<#di_lexical_block_file>[#loc1])
+ llvm.intr.dbg.label #di_label loc(#loc1)
llvm.return %arg0 : i32
} loc(fused<#di_subprogram>["caller"])
|
There was a problem hiding this comment.
LGTM % one comment. Thanks for working on this.
Regarding the semantic definition: I stumbled over this a while ago, and I agree that the current style of allowing Fused locations everywhere is very confusing and hard to map to LLVM in any reasonable way. If we can enforce that scopes are only attached to leaf locations, that would be a nice simplification.
Disclaimer: I need to think about this for a bit before I can state that this change would indeed be correct.
| llvmLoc = translateLoc(callLoc.getCallee(), scope, callerLoc); | ||
| llvmLoc = translateLoc(callLoc.getCallee(), nullptr, callerLoc); | ||
| if (!llvmLoc) | ||
| return callerLoc; // Fallback: Ignore callee if it has no debug scope. |
There was a problem hiding this comment.
Shouldn't we also add this to the cache, as we would do for the llvmLoc later on?
There was a problem hiding this comment.
oh right.. good point 🙏
|
Thanks for the reviews! Please feel free to just land this if it looks good. |
Done 🙂 Maybe consider to request commit access, given that you are contributing quite actively. https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access The main benefit of this is that potential buildbot failures will be reported to the person that landed a commit, such that necessary reverts or fixes can be shipped as soon as possible. |
| @@ -330,7 +330,10 @@ llvm::DILocation *DebugTranslation::translateLoc(Location loc, | |||
| if (auto callLoc = dyn_cast<CallSiteLoc>(loc)) { | |||
| // For callsites, the caller is fed as the inlinedAt for the callee. | |||
| auto *callerLoc = translateLoc(callLoc.getCaller(), scope, inlinedAt); | |||
| llvmLoc = translateLoc(callLoc.getCallee(), scope, callerLoc); | |||
| llvmLoc = translateLoc(callLoc.getCallee(), nullptr, callerLoc); | |||
There was a problem hiding this comment.
Hi, I'm working with triton and I'm not sure I fully understand this change - is this scope->nullptr really intentional? It looks like in no place you are propagating the scope? We've found that while we previously were getting the correct inlined location propagated for our debugging, we no longer are - now it propagates the parent scope instead of the inlined function location (ex: https://github.com/openai/triton/blob/main/python/test/unit/language/test_line_info.py#L143, we expect it to correctly be able to return the inlined function location, line 22, but it's now returning line 30, where the inlined function is called). Is it no longer supported that we see the location within the inlined function or is this a bug?
There was a problem hiding this comment.
Hi, thanks for checking, yes the nullptr here is intentional so that we don't push the parent scope into the callee scope (in case the callee didn't have a scope, and accidentally takes on the caller scope). This is actually to service the semantics change introduced here, which is that debug locations of callees are annotated in the callee location, not the overall callsite location (following the motivation mentioned in the PR description).
For example, what previously might look like
fused<#callee>[callsite("foo.mlir":2:4 at fused<#sp0>["foo.mlir":28:5])]
should now look like
callsite(fused<#callee>["foo.mlir":2:4] at fused<#sp0>["foo.mlir":28:5])
Can you double check that your llvm dialect IR is generated as expected?
There was a problem hiding this comment.
Thank you :) I was able to change it to reflect this
There's an issue in the translator today where, for a CallsiteLoc, if the callee does not have a DI scope (perhaps due to compile options or optimizations), it may get propagated the DI scope of its callsite's parent function, which will create a non-existent DILocation combining line & col number from one file, and the filename from another.
The root problem is we cannot propagate the parent scope when translating the callee location, as it no longer applies to inlined locations (see code diff and hopefully this will make sense).
To facilitate this, the importer is also changed so that callee scopes are fused with the callee FileLineCol loc, instead of on the Callsite loc itself. This comes with the benefit that we now have a symmetric Callsite loc representation. If we required the callee scope be always annotated on the Callsite loc, it would be hard for generic inlining passes to maintain that, since it would have to somehow understand the semantics of the fused metadata and pull it out while inlining.