Address feedback

bcardosolopes · bcardosolopes · commit 860a3b586159 · 2025-11-13T14:12:18.000-08:00
diff --git a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
@@ -283,7 +283,7 @@ class ModuleImport {
   void addDebugIntrinsic(llvm::CallInst *intrinsic);
 
   /// Similar to `addDebugIntrinsic`, but for debug records.
-  void addDebugRecord(llvm::DbgRecord *dr);
+  void addDebugRecord(llvm::DbgRecord *debugRecord);
 
   /// Converts the LLVM values for an intrinsic to mixed MLIR values and
   /// attributes for LLVM_IntrOpBase. Attributes correspond to LLVM immargs. The
@@ -350,15 +350,15 @@ class ModuleImport {
   LogicalResult processDebugIntrinsic(llvm::DbgVariableIntrinsic *dbgIntr,
                                       DominanceInfo &domInfo);
   /// Converts a single debug record.
-  LogicalResult processDebugRecord(llvm::DbgRecord &dr, DominanceInfo &domInfo);
+  LogicalResult processDebugRecord(llvm::DbgRecord &debugRecord,
+                                   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.
-  void processDebugOpArgumentsAndInsertionPt(
-      Location loc, DILocalVariableAttr &localVarAttr,
-      DIExpressionAttr &localExprAttr, Value &locVal, bool hasArgList,
-      bool isKillLocation,
+  std::tuple<DILocalVariableAttr, DIExpressionAttr, Value>
+  processDebugOpArgumentsAndInsertionPt(
+      Location loc, bool hasArgList, bool isKillLocation,
       llvm::function_ref<FailureOr<Value>()> convertArgOperandToValue,
       llvm::Value *address,
       llvm::PointerUnion<llvm::Value *, llvm::DILocalVariable *> variable,
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -524,9 +524,9 @@ void ModuleImport::addDebugIntrinsic(llvm::CallInst *intrinsic) {
   debugIntrinsics.insert(intrinsic);
 }
 
-void ModuleImport::addDebugRecord(llvm::DbgRecord *dr) {
-  if (!debugRecords.contains(dr))
-    debugRecords.insert(dr);
+void ModuleImport::addDebugRecord(llvm::DbgRecord *debugRecord) {
+  if (!debugRecords.contains(debugRecord))
+    debugRecords.insert(debugRecord);
 }
 
 static Attribute convertCGProfileModuleFlagValue(ModuleOp mlirModule,
@@ -2558,10 +2558,9 @@ LogicalResult ModuleImport::processInstruction(llvm::Instruction *inst) {
     return convertIntrinsic(intrinsic);
 
   // Capture instruction with attached debug markers for later processing.
-  if (inst->DebugMarker) {
-    for (llvm::DbgRecord &dr : inst->DebugMarker->getDbgRecordRange())
-      addDebugRecord(&dr);
-  }
+  if (inst->DebugMarker)
+    for (llvm::DbgRecord &debugRecord : inst->DebugMarker->getDbgRecordRange())
+      addDebugRecord(&debugRecord);
 
   // Convert all remaining LLVM instructions to MLIR operations.
   return convertInstruction(inst);
@@ -3026,7 +3025,7 @@ LogicalResult ModuleImport::processFunction(llvm::Function *func) {
   if (failed(processDebugIntrinsics()))
     return failure();
 
-  // Process the debug r that require a delayed conversion after
+  // Process the debug records that require a delayed conversion after
   // everything else was converted.
   if (failed(processDebugRecords()))
     return failure();
@@ -3044,8 +3043,8 @@ static bool isMetadataKillLocation(bool isKillLocation, llvm::Value *value) {
   return !isa<llvm::ValueAsMetadata>(nodeAsVal->getMetadata());
 }
 
-/// Ensure that the debug intrinsic is inserted right after its operand is
-/// defined. Otherwise, the operand might not necessarily dominate the
+/// Ensure that the debug intrinsic is inserted right after the operand
+/// definition. Otherwise, the operand might not necessarily dominate the
 /// intrinsic. If the defining operation is a terminator, insert the intrinsic
 /// into a dominated block.
 static LogicalResult setDebugIntrinsicBuilderInsertionPoint(
@@ -3080,41 +3079,40 @@ static LogicalResult setDebugIntrinsicBuilderInsertionPoint(
   return success();
 }
 
-void ModuleImport::processDebugOpArgumentsAndInsertionPt(
-    Location loc, DILocalVariableAttr &localVarAttr,
-    DIExpressionAttr &localExprAttr, Value &locVal, bool hasArgList,
-    bool isKillLocation,
+std::tuple<DILocalVariableAttr, DIExpressionAttr, Value>
+ModuleImport::processDebugOpArgumentsAndInsertionPt(
+    Location loc, bool hasArgList, bool isKillLocation,
     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;
+    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;
+    return {};
   // Drop debug intrinsics if the associated variable information cannot be
   // translated due to cyclic debug metadata.
   // TODO: Support cyclic debug metadata.
-  localVarAttr = matchLocalVariableAttr(variable);
+  DILocalVariableAttr localVarAttr = matchLocalVariableAttr(variable);
   if (!localVarAttr)
-    return;
+    return {};
   FailureOr<Value> argOperand = convertArgOperandToValue();
   if (failed(argOperand)) {
     emitError(loc) << "failed to convert a debug operand: " << diag(*address);
-    return;
+    return {};
   }
 
   if (setDebugIntrinsicBuilderInsertionPoint(builder, domInfo, *argOperand)
           .failed())
-    return;
+    return {};
 
-  localExprAttr = debugImporter->translateExpression(expression);
-  locVal = *argOperand;
+  return {localVarAttr, debugImporter->translateExpression(expression),
+          *argOperand};
 }
 
 LogicalResult
@@ -3127,99 +3125,92 @@ ModuleImport::processDebugIntrinsic(llvm::DbgVariableIntrinsic *dbgIntr,
     return success();
   };
 
-  DILocalVariableAttr localVariableAttr;
-  DIExpressionAttr locationExprAttr;
-  Value locVal;
   OpBuilder::InsertionGuard guard(builder);
   auto convertArgOperandToValue = [&]() {
     return convertMetadataValue(dbgIntr->getArgOperand(0));
   };
 
-  processDebugOpArgumentsAndInsertionPt(
-      loc, localVariableAttr, locationExprAttr, locVal, dbgIntr->hasArgList(),
-      dbgIntr->isKillLocation(), convertArgOperandToValue,
-      dbgIntr->getArgOperand(0), dbgIntr->getArgOperand(1),
-      dbgIntr->getExpression(), domInfo);
+  auto [localVariableAttr, locationExprAttr, locVal] =
+      processDebugOpArgumentsAndInsertionPt(
+          loc, dbgIntr->hasArgList(), dbgIntr->isKillLocation(),
+          convertArgOperandToValue, dbgIntr->getArgOperand(0),
+          dbgIntr->getArgOperand(1), dbgIntr->getExpression(), domInfo);
 
   if (!localVariableAttr)
     return emitUnsupportedWarning();
 
   if (!locVal) // Expected if localVariableAttr is present.
     return failure();
 
-  Operation *op =
-      llvm::TypeSwitch<llvm::DbgVariableIntrinsic *, Operation *>(dbgIntr)
-          .Case([&](llvm::DbgDeclareInst *) {
-            return LLVM::DbgDeclareOp::create(
-                builder, loc, locVal, localVariableAttr, locationExprAttr);
-          })
-          .Case([&](llvm::DbgValueInst *) {
-            return LLVM::DbgValueOp::create(
-                builder, loc, locVal, localVariableAttr, locationExprAttr);
-          });
+  Operation *op = nullptr;
+  if (isa<llvm::DbgDeclareInst>(dbgIntr))
+    op = LLVM::DbgDeclareOp::create(builder, loc, locVal, localVariableAttr,
+                                    locationExprAttr);
+  else if (isa<llvm::DbgValueInst>(dbgIntr))
+    op = LLVM::DbgValueOp::create(builder, loc, locVal, localVariableAttr,
+                                  locationExprAttr);
+  else
+    return emitUnsupportedWarning();
+
   mapNoResultOp(dbgIntr, op);
   setNonDebugMetadataAttrs(dbgIntr, op);
   return success();
 }
 
-LogicalResult ModuleImport::processDebugRecord(llvm::DbgRecord &dr,
+LogicalResult ModuleImport::processDebugRecord(llvm::DbgRecord &debugRecord,
                                                DominanceInfo &domInfo) {
-  Location loc = translateLoc(dr.getDebugLoc());
+  Location loc = translateLoc(debugRecord.getDebugLoc());
   auto emitUnsupportedWarning = [&]() {
     if (!emitExpensiveWarnings)
       return success();
     std::string options;
     llvm::raw_string_ostream optionsStream(options);
-    dr.print(optionsStream);
+    debugRecord.print(optionsStream);
     emitWarning(loc) << "unhandled debug record " << optionsStream.str();
     return success();
   };
 
   OpBuilder::InsertionGuard guard(builder);
-  if (auto *dbgVar = dyn_cast<llvm::DbgVariableRecord>(&dr)) {
-    DILocalVariableAttr localVariableAttr;
-    DIExpressionAttr locationExprAttr;
-    Value locVal;
-    auto convertArgOperandToValue = [&]() -> FailureOr<Value> {
-      llvm::Value *value = dbgVar->getAddress();
-
-      // Return the mapped value if it has been converted before.
-      auto it = valueMapping.find(value);
-      if (it != valueMapping.end())
-        return it->getSecond();
-
-      // Convert constants such as immediate values that have no mapping yet.
-      if (auto *constant = dyn_cast<llvm::Constant>(value))
-        return convertConstantExpr(constant);
-      return failure();
-    };
+  auto *dbgVar = dyn_cast<llvm::DbgVariableRecord>(&debugRecord);
+  if (!dbgVar)
+    return emitUnsupportedWarning();
 
-    processDebugOpArgumentsAndInsertionPt(
-        loc, localVariableAttr, locationExprAttr, locVal, dbgVar->hasArgList(),
-        dbgVar->isKillLocation(), convertArgOperandToValue,
-        dbgVar->getAddress(), dbgVar->getVariable(), dbgVar->getExpression(),
-        domInfo);
+  auto convertArgOperandToValue = [&]() -> FailureOr<Value> {
+    llvm::Value *value = dbgVar->getAddress();
 
-    if (!localVariableAttr)
-      return emitUnsupportedWarning();
+    // Return the mapped value if it has been converted before.
+    auto it = valueMapping.find(value);
+    if (it != valueMapping.end())
+      return it->getSecond();
 
-    if (!locVal) // Expected if localVariableAttr is present.
-      return failure();
+    // Convert constants such as immediate values that have no mapping yet.
+    if (auto *constant = dyn_cast<llvm::Constant>(value))
+      return convertConstantExpr(constant);
+    return failure();
+  };
 
-    if (dbgVar->isDbgDeclare())
-      LLVM::DbgDeclareOp::create(builder, loc, locVal, localVariableAttr,
-                                 locationExprAttr);
-    else if (dbgVar->isDbgValue())
-      LLVM::DbgValueOp::create(builder, loc, locVal, localVariableAttr,
-                               locationExprAttr);
-    else // isDbgAssign
-      return emitUnsupportedWarning();
+  auto [localVariableAttr, locationExprAttr, locVal] =
+      processDebugOpArgumentsAndInsertionPt(
+          loc, dbgVar->hasArgList(), dbgVar->isKillLocation(),
+          convertArgOperandToValue, dbgVar->getAddress(), dbgVar->getVariable(),
+          dbgVar->getExpression(), domInfo);
 
-    // FIXME: Nothing to map given the source is an LLVM attribute?
-    return success();
-  }
+  if (!localVariableAttr)
+    return emitUnsupportedWarning();
 
-  return emitUnsupportedWarning();
+  if (!locVal) // Expected if localVariableAttr is present.
+    return failure();
+
+  if (dbgVar->isDbgDeclare())
+    LLVM::DbgDeclareOp::create(builder, loc, locVal, localVariableAttr,
+                               locationExprAttr);
+  else if (dbgVar->isDbgValue())
+    LLVM::DbgValueOp::create(builder, loc, locVal, localVariableAttr,
+                             locationExprAttr);
+  else // isDbgAssign
+    return emitUnsupportedWarning();
+
+  return success();
 }
 
 LogicalResult ModuleImport::processDebugIntrinsics() {
@@ -3234,8 +3225,8 @@ LogicalResult ModuleImport::processDebugIntrinsics() {
 
 LogicalResult ModuleImport::processDebugRecords() {
   DominanceInfo domInfo;
-  for (llvm::DbgRecord *dr : debugRecords) {
-    if (failed(processDebugRecord(*dr, domInfo)))
+  for (llvm::DbgRecord *debugRecord : debugRecords) {
+    if (failed(processDebugRecord(*debugRecord, domInfo)))
       return failure();
   }
   debugRecords.clear();
diff --git a/mlir/test/Target/LLVMIR/Import/debug-info-records.ll b/mlir/test/Target/LLVMIR/Import/debug-info-records.ll
@@ -18,22 +18,16 @@ define void @func_no_debug() {
 ; CHECK: llvm.func @func_with_debug(%[[ARG0:.*]]: i64
 define void @func_with_debug(i64 %0) !dbg !3 {
 
-  ; CHECK: llvm.intr.dbg.value #di_local_variable = %[[ARG0]] : i64
-  ; CHECK: llvm.intr.dbg.value #di_local_variable1 #llvm.di_expression<[DW_OP_LLVM_fragment(0, 1)]> = %[[ARG0]] : i64
+  ; CHECK: llvm.intr.dbg.value #di_local_variable{{.*}} = %[[ARG0]] : i64
+  ; CHECK: llvm.intr.dbg.value #di_local_variable{{.*}} #llvm.di_expression<[DW_OP_LLVM_fragment(0, 1)]> = %[[ARG0]] : i64
   ; CHECK: %[[CST:.*]] = llvm.mlir.constant(1 : i32) : i32
   ; CHECK: %[[ADDR:.*]] = llvm.alloca %[[CST]] x i64
-  ; CHECK: llvm.intr.dbg.declare #di_local_variable2 #llvm.di_expression<[DW_OP_deref, DW_OP_LLVM_convert(4, DW_ATE_signed)]> = %[[ADDR]] : !llvm.ptr
+  ; CHECK: llvm.intr.dbg.declare #di_local_variable{{.*}} #llvm.di_expression<[DW_OP_deref, DW_OP_LLVM_convert(4, DW_ATE_signed)]> = %[[ADDR]] : !llvm.ptr
   %2 = alloca i64, align 8, !dbg !19
     #dbg_value(i64 %0, !20, !DIExpression(DW_OP_LLVM_fragment, 0, 1), !22)
     #dbg_declare(ptr %2, !23, !DIExpression(DW_OP_deref, DW_OP_LLVM_convert, 4, DW_ATE_signed), !25)
     #dbg_value(i64 %0, !26, !DIExpression(), !27)
   call void @func_no_debug(), !dbg !28
-  call void @func_no_debug(), !dbg !28
-  call void @func_no_debug(), !dbg !29
-  call void @func_no_debug(), !dbg !30
-  call void @func_no_debug(), !dbg !30
-  call void @func_no_debug(), !dbg !31
-  call void @func_no_debug(), !dbg !32
   %3 = add i64 %0, %0, !dbg !32
   ret void, !dbg !37
 }
@@ -74,9 +68,6 @@ define void @empty_types() !dbg !38 {
 !26 = !DILocalVariable(scope: !24)
 !27 = !DILocation(line: 109, column: 3, scope: !3)
 !28 = !DILocation(line: 1, column: 2, scope: !3)
-!29 = !DILocation(line: 10, column: 10, scope: !3)
-!30 = !DILocation(line: 5, column: 6, scope: !3)
-!31 = !DILocation(line: 1, column: 1, scope: !3)
 !32 = !DILocation(line: 2, column: 4, scope: !33, inlinedAt: !36)
 !33 = distinct !DISubprogram(name: "callee", scope: !13, file: !1, type: !34, spFlags: DISPFlagDefinition, unit: !0)
 !34 = !DISubroutineType(types: !35)
