[mlir][llvm] Handle debug record import edge cases #168774
Merged
+97
−58
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gysit
commented
Nov 19, 2025
| // translated due to cyclic debug metadata. | ||
| // TODO: Support cyclic debug metadata. | ||
| // Drop debug intrinsics if the associated debug information cannot be | ||
| // translated due to an unsupported construct. |
Contributor
Author
There was a problem hiding this comment.
The cyclic debug metadata is now supported but there may be other unsupported constructs.
gysit
commented
Nov 19, 2025
| // Drop kill location debug records with a null address operand, which cannot | ||
| // be converted to poison as the type cannot be reconstructed. | ||
| // TODO: Support this case. | ||
| if (!dbgVar->getAddress()) |
Contributor
Author
There was a problem hiding this comment.
I believe the logic in isMetadataKillLocation is not needed here since getAddress() directly returns a Value or nullptr if the Value is an empty Metadata node (we support only the cases where a value is returned).
🐧 Linux x64 Test Results
|
Member
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: Tobias Gysi (gysit) ChangesThis commit enables the direct import of debug records by default and fixes issues with two edge cases:
This is a follow-up to: Full diff: https://github.com/llvm/llvm-project/pull/168774.diff 5 Files Affected:
diff --git a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
index a4a7df985b681..75459821c19d1 100644
--- a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
+++ b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
@@ -358,7 +358,7 @@ class ModuleImport {
/// used by these operations.
std::tuple<DILocalVariableAttr, DIExpressionAttr, Value>
processDebugOpArgumentsAndInsertionPt(
- Location loc, bool hasArgList, bool isKillLocation,
+ Location loc,
llvm::function_ref<FailureOr<Value>()> convertArgOperandToValue,
llvm::Value *address,
llvm::PointerUnion<llvm::Value *, llvm::DILocalVariable *> variable,
diff --git a/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp b/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
index ba80f6294bd9b..5be33c4c78138 100644
--- a/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
+++ b/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
@@ -79,7 +79,7 @@ void registerFromLLVMIRTranslation() {
// Now that the translation supports importing debug records directly,
// make it the default, but allow the user to override to old behavior.
- if (!convertDebugRecToIntrinsics)
+ if (convertDebugRecToIntrinsics)
llvmModule->convertFromNewDbgValues();
return translateLLVMIRToModule(
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index 4d63e0534358f..5bcc52fd2bbae 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -3047,10 +3047,12 @@ LogicalResult ModuleImport::processFunction(llvm::Function *func) {
return success();
}
-/// Checks if a kill location holds metadata instead of an SSA value.
-static bool isMetadataKillLocation(bool isKillLocation, llvm::Value *value) {
- if (!isKillLocation)
+/// Checks if `dbgIntr` is a kill location that holds metadata instead of an SSA
+/// value.
+static bool isMetadataKillLocation(llvm::DbgVariableIntrinsic *dbgIntr) {
+ if (!dbgIntr->isKillLocation())
return false;
+ llvm::Value *value = dbgIntr->getArgOperand(0);
auto *nodeAsVal = dyn_cast<llvm::MetadataAsValue>(value);
if (!nodeAsVal)
return false;
@@ -3095,23 +3097,13 @@ static LogicalResult setDebugIntrinsicBuilderInsertionPoint(
std::tuple<DILocalVariableAttr, DIExpressionAttr, Value>
ModuleImport::processDebugOpArgumentsAndInsertionPt(
- Location loc, bool hasArgList, bool isKillLocation,
+ Location loc,
llvm::function_ref<FailureOr<Value>()> convertArgOperandToValue,
llvm::Value *address,
llvm::PointerUnion<llvm::Value *, llvm::DILocalVariable *> variable,
llvm::DIExpression *expression, DominanceInfo &domInfo) {
- // Drop debug intrinsics with arg lists.
- // TODO: Support debug intrinsics that have arg lists.
- if (hasArgList)
- return {};
- // Kill locations can have metadata nodes as location operand. This
- // cannot be converted to poison as the type cannot be reconstructed.
- // TODO: find a way to support this case.
- if (isMetadataKillLocation(isKillLocation, address))
- return {};
- // Drop debug intrinsics if the associated variable information cannot be
- // translated due to cyclic debug metadata.
- // TODO: Support cyclic debug metadata.
+ // Drop debug intrinsics if the associated debug information cannot be
+ // translated due to an unsupported construct.
DILocalVariableAttr localVarAttr = matchLocalVariableAttr(variable);
if (!localVarAttr)
return {};
@@ -3144,10 +3136,21 @@ ModuleImport::processDebugIntrinsic(llvm::DbgVariableIntrinsic *dbgIntr,
return convertMetadataValue(dbgIntr->getArgOperand(0));
};
+ // Drop debug intrinsics with an argument list.
+ // TODO: Support this case.
+ if (dbgIntr->hasArgList())
+ return emitUnsupportedWarning();
+
+ // Drop debug intrinsics with kill locations that have metadata nodes as
+ // location operand, which cannot be converted to poison as the type cannot be
+ // reconstructed.
+ // TODO: Support this case.
+ if (isMetadataKillLocation(dbgIntr))
+ return emitUnsupportedWarning();
+
auto [localVariableAttr, locationExprAttr, locVal] =
processDebugOpArgumentsAndInsertionPt(
- loc, dbgIntr->hasArgList(), dbgIntr->isKillLocation(),
- convertArgOperandToValue, dbgIntr->getArgOperand(0),
+ loc, convertArgOperandToValue, dbgIntr->getArgOperand(0),
dbgIntr->getArgOperand(1), dbgIntr->getExpression(), domInfo);
if (!localVariableAttr)
@@ -3203,11 +3206,21 @@ LogicalResult ModuleImport::processDebugRecord(llvm::DbgRecord &debugRecord,
return failure();
};
+ // Drop debug records with an argument list.
+ // TODO: Support this case.
+ if (dbgVar->hasArgList())
+ return emitUnsupportedWarning();
+
+ // Drop kill location debug records with a null address operand, which cannot
+ // be converted to poison as the type cannot be reconstructed.
+ // TODO: Support this case.
+ if (!dbgVar->getAddress())
+ return emitUnsupportedWarning();
+
auto [localVariableAttr, locationExprAttr, locVal] =
processDebugOpArgumentsAndInsertionPt(
- loc, dbgVar->hasArgList(), dbgVar->isKillLocation(),
- convertArgOperandToValue, dbgVar->getAddress(), dbgVar->getVariable(),
- dbgVar->getExpression(), domInfo);
+ loc, convertArgOperandToValue, dbgVar->getAddress(),
+ dbgVar->getVariable(), dbgVar->getExpression(), domInfo);
if (!localVariableAttr)
return emitUnsupportedWarning();
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info.ll b/mlir/test/Target/LLVMIR/Import/debug-info.ll
index 61376b8f648ec..0d7bad53bf087 100644
--- a/mlir/test/Target/LLVMIR/Import/debug-info.ll
+++ b/mlir/test/Target/LLVMIR/Import/debug-info.ll
@@ -1,4 +1,4 @@
-; RUN: mlir-translate -import-llvm -mlir-print-debuginfo -split-input-file %s | FileCheck %s
+; RUN: mlir-translate -import-llvm -convert-debug-rec-to-intrinsics -mlir-print-debuginfo -split-input-file %s | FileCheck %s
; CHECK: #[[$UNKNOWN_LOC:.+]] = loc(unknown)
diff --git a/mlir/test/Target/LLVMIR/Import/import-failure.ll b/mlir/test/Target/LLVMIR/Import/import-failure.ll
index d48be66f2063e..8fe09892c2b49 100644
--- a/mlir/test/Target/LLVMIR/Import/import-failure.ll
+++ b/mlir/test/Target/LLVMIR/Import/import-failure.ll
@@ -1,16 +1,14 @@
; RUN: not mlir-translate -import-llvm -emit-expensive-warnings -split-input-file %s 2>&1 -o /dev/null | FileCheck %s
-; Check that debug intrinsics with an unsupported argument are dropped.
-
-declare void @llvm.dbg.value(metadata, metadata, metadata)
+; Check that debug records with an unsupported argument are dropped.
; CHECK: import-failure.ll
-; CHECK-SAME: warning: dropped intrinsic: tail call void @llvm.dbg.value(metadata !DIArgList(i64 %{{.*}}, i64 undef), metadata !3, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 1, DW_OP_mul, DW_OP_plus, DW_OP_stack_value))
+; CHECK-SAME: warning: unhandled debug record #dbg_value(!DIArgList(i64 %{{.*}}, i64 undef), !{{.*}}, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 1, DW_OP_mul, DW_OP_plus, DW_OP_stack_value), !{{.*}})
; CHECK: import-failure.ll
-; CHECK-SAME: warning: dropped intrinsic: tail call void @llvm.dbg.value(metadata !6, metadata !3, metadata !DIExpression())
+; CHECK-SAME: warning: unhandled debug record #dbg_value(!{{.*}}, !{{.*}}, !DIExpression(), !{{.*}})
define void @unsupported_argument(i64 %arg1) {
- tail call void @llvm.dbg.value(metadata !DIArgList(i64 %arg1, i64 undef), metadata !3, metadata !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 1, DW_OP_mul, DW_OP_plus, DW_OP_stack_value)), !dbg !5
- tail call void @llvm.dbg.value(metadata !6, metadata !3, metadata !DIExpression()), !dbg !5
+ #dbg_value(!DIArgList(i64 %arg1, i64 undef), !3, !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_constu, 1, DW_OP_mul, DW_OP_plus, DW_OP_stack_value), !5)
+ #dbg_value(!6, !3, !DIExpression(), !5)
ret void
}
|
gysit
force-pushed
the
users/gysit/fix-debug-rec-import
branch
from
6ce310d to
8fda2cf
Compare
definelicht
approved these changes
Nov 20, 2025
Contributor
definelicht
left a comment
definelicht
left a comment
There was a problem hiding this comment.
LGTM with my limited context 🙂
Comment on lines
353
to
355
| /// Emits a warning for an unsupported debug records. | ||
| LogicalResult emitUnsupportedDebugRecordWarning(llvm::DbgRecord &debugRecord, | ||
| Location loc); |
Contributor
There was a problem hiding this comment.
Why does this live in the header, isn't it only used in ModuleImport.cpp? 👀
Contributor
Author
There was a problem hiding this comment.
It uses a private member variable (emitExpensiveWarnings) of the ModuleImport class.
xlauko
reviewed
Nov 20, 2025
| debugRecord.print(optionsStream); | ||
| emitWarning(loc) << "unhandled debug record " << optionsStream.str(); | ||
| LogicalResult | ||
| ModuleImport::emitUnsupportedDebugRecordWarning(llvm::DbgRecord &debugRecord, |
Contributor
There was a problem hiding this comment.
Does this need to be method? Also it is a bit inconsistent with:
emitUnsupportedWarning which is inline lambda.
Contributor
Author
There was a problem hiding this comment.
It is used in two places and wanted to avoid the code duplication (it used a member variable). I guess two labmdas are fine. With regards to the intrinsic code path, note that is about to disappear.
Contributor
Author
There was a problem hiding this comment.
I ended up having two lambdas with slightly different messages.
This commit enables the direct import of debug records by default and fixes issues with two edge cases: - Detect early on if the address operand is an argument list (calling getAddress() for argument lists asserts) - Use getAddress() to check if the address operand is null, which means the address operand is an empty metadata node, which currently is not supported. - Add support for debug label records. This is a follow-up to: #167812
gysit
force-pushed
the
users/gysit/fix-debug-rec-import
branch
from
8fda2cf to
35c1bfd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit enables the direct import of debug records by default and fixes issues with two edge cases:
This is a follow-up to:
#167812