[mlir][llvm] Handle debug record import edge cases (#168774)

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

Author: gysit

mlir/include/mlir/Target/LLVMIR/ModuleImport.h

@@ -282,8 +282,9 @@ class ModuleImport {
   /// after the function conversion has finished.
   void addDebugIntrinsic(llvm::CallInst *intrinsic);
 
-  /// Similar to `addDebugIntrinsic`, but for debug records.
-  void addDebugRecord(llvm::DbgRecord *debugRecord);
+  /// Adds a debug record to the list of debug records that need to be imported
+  /// after the function conversion has finished.
+  void addDebugRecord(llvm::DbgVariableRecord *dbgRecord);
 
   /// Converts the LLVM values for an intrinsic to mixed MLIR values and
   /// attributes for LLVM_IntrOpBase. Attributes correspond to LLVM immargs. The
@@ -343,22 +344,22 @@ class ModuleImport {
   /// Converts all debug intrinsics in `debugIntrinsics`. Assumes that the
   /// function containing the intrinsics has been fully converted to MLIR.
   LogicalResult processDebugIntrinsics();
-  /// Converts all debug records in `debugRecords`. Assumes that the
+  /// Converts all debug records in `dbgRecords`. Assumes that the
   /// function containing the record has been fully converted to MLIR.
   LogicalResult processDebugRecords();
   /// Converts a single debug intrinsic.
   LogicalResult processDebugIntrinsic(llvm::DbgVariableIntrinsic *dbgIntr,
                                       DominanceInfo &domInfo);
   /// Converts a single debug record.
-  LogicalResult processDebugRecord(llvm::DbgRecord &debugRecord,
+  LogicalResult processDebugRecord(llvm::DbgVariableRecord &dbgRecord,
                                    DominanceInfo &domInfo);
   /// Process arguments for declare/value operation insertion. `localVarAttr`
   /// and `localExprAttr` are the attained attributes after importing the debug
   /// variable and expressions. This also sets the builder insertion point to be
   /// 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,
@@ -508,7 +509,7 @@ class ModuleImport {
   SetVector<llvm::Instruction *> debugIntrinsics;
   /// Function-local list of debug records that need to be imported after the
   /// function conversion has finished.
-  SetVector<llvm::DbgRecord *> debugRecords;
+  SetVector<llvm::DbgVariableRecord *> dbgRecords;
   /// Mapping between LLVM alias scope and domain metadata nodes and
   /// attributes in the LLVM dialect corresponding to these nodes.
   DenseMap<const llvm::MDNode *, Attribute> aliasScopeMapping;

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(

mlir/lib/Target/LLVMIR/ModuleImport.cpp

@@ -524,9 +524,9 @@ void ModuleImport::addDebugIntrinsic(llvm::CallInst *intrinsic) {
   debugIntrinsics.insert(intrinsic);
 }
 
-void ModuleImport::addDebugRecord(llvm::DbgRecord *debugRecord) {
-  if (!debugRecords.contains(debugRecord))
-    debugRecords.insert(debugRecord);
+void ModuleImport::addDebugRecord(llvm::DbgVariableRecord *dbgRecord) {
+  if (!dbgRecords.contains(dbgRecord))
+    dbgRecords.insert(dbgRecord);
 }
 
 static Attribute convertCGProfileModuleFlagValue(ModuleOp mlirModule,
@@ -2557,10 +2557,40 @@ LogicalResult ModuleImport::processInstruction(llvm::Instruction *inst) {
   if (auto *intrinsic = dyn_cast<llvm::IntrinsicInst>(inst))
     return convertIntrinsic(intrinsic);
 
-  // Capture instruction with attached debug markers for later processing.
-  if (inst->DebugMarker)
-    for (llvm::DbgRecord &debugRecord : inst->DebugMarker->getDbgRecordRange())
-      addDebugRecord(&debugRecord);
+  // Process debug records attached to this instruction. Debug variable records
+  // are stored for later processing after all SSA values are converted, while
+  // debug label records can be converted immediately.
+  if (inst->DebugMarker) {
+    for (llvm::DbgRecord &dbgRecord : inst->DebugMarker->getDbgRecordRange()) {
+      // Store debug variable records for later processing.
+      if (auto *dbgVariableRecord =
+              dyn_cast<llvm::DbgVariableRecord>(&dbgRecord)) {
+        addDebugRecord(dbgVariableRecord);
+        continue;
+      }
+      Location loc = translateLoc(dbgRecord.getDebugLoc());
+      auto emitUnsupportedWarning = [&]() -> LogicalResult {
+        if (!emitExpensiveWarnings)
+          return success();
+        std::string options;
+        llvm::raw_string_ostream optionsStream(options);
+        dbgRecord.print(optionsStream);
+        emitWarning(loc) << "unhandled debug record " << optionsStream.str();
+        return success();
+      };
+      // Convert the debug label records in-place.
+      if (auto *dbgLabelRecord = dyn_cast<llvm::DbgLabelRecord>(&dbgRecord)) {
+        DILabelAttr labelAttr =
+            debugImporter->translate(dbgLabelRecord->getLabel());
+        if (!labelAttr)
+          return emitUnsupportedWarning();
+        LLVM::DbgLabelOp::create(builder, loc, labelAttr);
+        continue;
+      }
+      // Warn if an unsupported debug record is encountered.
+      return emitUnsupportedWarning();
+    }
+  }
 
   // Convert all remaining LLVM instructions to MLIR operations.
   return convertInstruction(inst);
@@ -3047,10 +3077,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 +3127,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 +3166,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)
@@ -3171,26 +3204,35 @@ ModuleImport::processDebugIntrinsic(llvm::DbgVariableIntrinsic *dbgIntr,
   return success();
 }
 
-LogicalResult ModuleImport::processDebugRecord(llvm::DbgRecord &debugRecord,
-                                               DominanceInfo &domInfo) {
-  Location loc = translateLoc(debugRecord.getDebugLoc());
-  auto emitUnsupportedWarning = [&]() {
+LogicalResult
+ModuleImport::processDebugRecord(llvm::DbgVariableRecord &dbgRecord,
+                                 DominanceInfo &domInfo) {
+  OpBuilder::InsertionGuard guard(builder);
+  Location loc = translateLoc(dbgRecord.getDebugLoc());
+  auto emitUnsupportedWarning = [&]() -> LogicalResult {
     if (!emitExpensiveWarnings)
       return success();
     std::string options;
     llvm::raw_string_ostream optionsStream(options);
-    debugRecord.print(optionsStream);
-    emitWarning(loc) << "unhandled debug record " << optionsStream.str();
+    dbgRecord.print(optionsStream);
+    emitWarning(loc) << "unhandled debug variable record "
+                     << optionsStream.str();
     return success();
   };
 
-  OpBuilder::InsertionGuard guard(builder);
-  auto *dbgVar = dyn_cast<llvm::DbgVariableRecord>(&debugRecord);
-  if (!dbgVar)
+  // Drop debug records with an argument list.
+  // TODO: Support this case.
+  if (dbgRecord.hasArgList())
+    return emitUnsupportedWarning();
+
+  // Drop all other debug records with a address operand that cannot be
+  // converted to an SSA value such as an empty metadata node.
+  // TODO: Support this case.
+  if (!dbgRecord.getAddress())
     return emitUnsupportedWarning();
 
   auto convertArgOperandToValue = [&]() -> FailureOr<Value> {
-    llvm::Value *value = dbgVar->getAddress();
+    llvm::Value *value = dbgRecord.getAddress();
 
     // Return the mapped value if it has been converted before.
     auto it = valueMapping.find(value);
@@ -3205,20 +3247,19 @@ LogicalResult ModuleImport::processDebugRecord(llvm::DbgRecord &debugRecord,
 
   auto [localVariableAttr, locationExprAttr, locVal] =
       processDebugOpArgumentsAndInsertionPt(
-          loc, dbgVar->hasArgList(), dbgVar->isKillLocation(),
-          convertArgOperandToValue, dbgVar->getAddress(), dbgVar->getVariable(),
-          dbgVar->getExpression(), domInfo);
+          loc, convertArgOperandToValue, dbgRecord.getAddress(),
+          dbgRecord.getVariable(), dbgRecord.getExpression(), domInfo);
 
   if (!localVariableAttr)
     return emitUnsupportedWarning();
 
   if (!locVal) // Expected if localVariableAttr is present.
     return failure();
 
-  if (dbgVar->isDbgDeclare())
+  if (dbgRecord.isDbgDeclare())
     LLVM::DbgDeclareOp::create(builder, loc, locVal, localVariableAttr,
                                locationExprAttr);
-  else if (dbgVar->isDbgValue())
+  else if (dbgRecord.isDbgValue())
     LLVM::DbgValueOp::create(builder, loc, locVal, localVariableAttr,
                              locationExprAttr);
   else // isDbgAssign
@@ -3239,11 +3280,10 @@ LogicalResult ModuleImport::processDebugIntrinsics() {
 
 LogicalResult ModuleImport::processDebugRecords() {
   DominanceInfo domInfo;
-  for (llvm::DbgRecord *debugRecord : debugRecords) {
-    if (failed(processDebugRecord(*debugRecord, domInfo)))
+  for (llvm::DbgVariableRecord *dbgRecord : dbgRecords)
+    if (failed(processDebugRecord(*dbgRecord, domInfo)))
       return failure();
-  }
-  debugRecords.clear();
+  dbgRecords.clear();
   return success();
 }
 

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 variable 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 variable 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
 }