[RemoveDIs][MLIR] Don't process debug records in the LLVM-IR translator

[SLTozer]

We are almost ready to enable the use of debug records everywhere in LLVM by default; part of the prep-work for this means ensuring that every tool supports them. Every tool in the llvm/ project supports them, front-ends that use the DIBuilder will support them, and as far as I can tell, the only other tool in the LLVM repo that needs to support them but doesn't is mlir-translate. This patch trivially unblocks them by converting from debug records to debug intrinsics before translating a module.

The steps for adding real support for debug records are probably quite simple:

1. Iterate over debug records at the same time as iterating over instructions. The list of debug records that precede an instruction I are obtained from I.getDbgRecordRange(), so these can be processed immediately before processing I. Debug records have all the same fields and methods as debug intrinsics, so porting logic should generally be trivial.
2. Build debug records using the DIBuilder rather than the IRBuilder; the latter will only produce debug intrinsics, while the former will produce either debug records or debug intrinsics according to the debug info format of the target module.

These are changes that I may be able to implement in time, but for the moment since there's not a huge advantage to support for debug records AFAICT (since we're translating out of the IR module, meaning there's no advantage to records over intrinsics), the simple pre-translate conversion should suffice for now.

[llvmbot]

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

@llvm/pr-subscribers-mlir

Author: Stephen Tozer (SLTozer)

Changes

We are almost ready to enable the use of debug records everywhere in LLVM by default; part of the prep-work for this means ensuring that every tool supports them. Every tool in the llvm/ project supports them, front-ends that use the DIBuilder will support them, and as far as I can tell, the only other tool in the LLVM repo that needs to support them but doesn't is mlir-translate. This patch trivially unblocks them by converting from debug records to debug intrinsics before translating a module.

The steps for adding real support for debug records are probably quite simple:

1. Iterate over debug records at the same time as iterating over instructions. The list of debug records that precede an instruction I are obtained from I.getDbgRecordRange(), so these can be processed immediately before processing I. Debug records have all the same fields and methods as debug intrinsics, so porting logic should generally be trivial.
2. Build debug records using the DIBuilder rather than the IRBuilder; the latter will only produce debug intrinsics, while the former will produce either debug records or debug intrinsics according to the debug info format of the target module.

These are changes that I may be able to implement in time, but for the moment since there's not a huge advantage to support for debug records AFAICT (since we're translating out of the IR module, meaning there's no advantage to records over intrinsics), the simple pre-translate conversion should suffice for now.

---

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

2 Files Affected:

• (modified) mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp (+4)
• (modified) mlir/test/Target/LLVMIR/Import/import-failure.ll (+4-4)

diff --git a/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp b/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
index 101add70c51e29..7e2da1e0303c74 100644
--- a/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
+++ b/mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
@@ -56,6 +56,10 @@ void registerFromLLVMIRTranslation() {
         if (llvm::verifyModule(*llvmModule, &llvm::errs()))
           return nullptr;
 
+        // Debug records are not currently supported in the LLVM IR translator.
+        if (llvmModule->IsNewDbgInfoFormat)
+          llvmModule->convertFromNewDbgValues();
+
         return translateLLVMIRToModule(std::move(llvmModule), context,
                                        emitExpensiveWarnings,
                                        dropDICompositeTypeElements);
diff --git a/mlir/test/Target/LLVMIR/Import/import-failure.ll b/mlir/test/Target/LLVMIR/Import/import-failure.ll
index 3f4efab70e1c02..9c24ed2b75746b 100644
--- a/mlir/test/Target/LLVMIR/Import/import-failure.ll
+++ b/mlir/test/Target/LLVMIR/Import/import-failure.ll
@@ -64,12 +64,12 @@ define void @unhandled_intrinsic() gc "example" {
 declare void @llvm.dbg.value(metadata, metadata, metadata)
 
 ; CHECK:      import-failure.ll
-; CHECK-SAME: warning: dropped intrinsic: 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: 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:      import-failure.ll
-; CHECK-SAME: warning: dropped intrinsic: call void @llvm.dbg.value(metadata !6, metadata !3, metadata !DIExpression())
+; CHECK-SAME: warning: dropped intrinsic: tail call void @llvm.dbg.value(metadata !6, metadata !3, metadata !DIExpression())
 define void @unsupported_argument(i64 %arg1) {
-  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
-  call void @llvm.dbg.value(metadata !6, metadata !3, metadata !DIExpression()), !dbg !5
+  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
   ret void
 }
 

[SLTozer]

The code and checks have been updated here because internally to LLVM we're deleting and recreating these intrinsics; although it seems like it should be a no-op, the builder will eagerly try to make the new intrinsics tail calls, hence we get the tail prefix after the otherwise-opaque roundtrip.

[Dinistro]

LGTM, thanks for working on this.
Enhancing the import to support the new debug format properly will probably go hand-in-hand with changes in the way we model debug metadata in MLIR's dialect. Thus, it will take substantial amount of time until we get there.

[gysit]

Thanks LGTM!

[zyx-billy]

Thank you!

Enhancing the import to support the new debug format properly will probably go hand-in-hand with changes in the way we model debug metadata in MLIR's dialect.

Are there any discussions on changing how debug ops are modeled in the LLVM dialect too? Or just that this might happen at some point and until then it's not worth updating the import.

[gysit]

Are there any discussions on changing how debug ops are modeled in the LLVM dialect too? Or just that this might happen at some point and until then it's not worth updating the import.

I am not aware of an active discussion but may have missed it. Normally adoption of new features is more or less need driven and not too fast. Are you familiar with the new representation? I wonder if we could use the debug locations directly to store the information.

[zyx-billy]

oh I see. I wasn't proposing any changes just wondering if there were any 😂 I haven't looked into the code yet, but it sounds like there's a new storage inside instructions, so yeah location or attributes may work, but we can have that discussion on discourse when it's needed (which I don't see happening too soon either).