[PATCH] D136173: [DebugInfo] Add function to test debug values for equivalence

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

On the whole this looks good to me but I do have a few nits and questions.

1. Could we do the comparison in a way that doesn't involve generating new unused metadata? For example, by just constructing an array of the `DIExpression` elements for each (no call to `DIExpression::get`) and comparing them elementwise?

2. I think the change to `DIExpression::isValid()` could come as a separate patch - and possibly the change to `DbgValueLocEntry` too. wdyt?



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2744-2746
+  /// If \p Expr is a non-variadic location (i.e. one that does not contain
+  /// DW_OP_LLVM_arg), returns Expr converted to variadic form by adding a
+  /// leading [DW_OP_LLVM_arg, 0] to the expression; otherwise returns Expr.
----------------
nit: suggestion above or even "If \p Expr is non-variadic (i.e. ...."


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2761
+  /// arguments, but apply to the second debug value.
+  static bool isEqualDebugValue(const DIExpression *FirstExpr,
+                                bool FirstIndirect,
----------------
Since this function doesn't take into account the value or DIArgList contents I think it should be called `isEqualExpression` or something. Wdyt?


================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:680-681
+    return false;
+  if (getNumDebugOperands() != Other.getNumDebugOperands())
+    return false;
+  for (unsigned OpIdx = 0; OpIdx < getNumDebugOperands(); ++OpIdx)
----------------
Could be beneficial to hoist this up to come after `isIdentical` check as it is a low-cost check?


================
Comment at: llvm/test/DebugInfo/Generic/merge-equivalent-ranges.ll:21
+  %CUID.addr = alloca i32, align 4
+  call void @llvm.dbg.value(metadata i32 %CUID, metadata !16, metadata !DIExpression(DW_OP_stack_value)), !dbg !17
+  store i32 %CUID, ptr %CUID.addr, align 4
----------------
What happens to the dbg.value above?


================
Comment at: llvm/unittests/IR/MetadataTest.cpp:3022-3027
+  EXPECT_NE_DEBUG_VALUE(GET_EXPR(dwarf::DW_OP_LLVM_arg, 0, dwarf::DW_OP_constu,
+                                 8, dwarf::DW_OP_plus),
+                        false,
+                        GET_EXPR(dwarf::DW_OP_constu, 8, dwarf::DW_OP_LLVM_arg,
+                                 0, dwarf::DW_OP_plus),
+                        false);
----------------
Maybe worth adding a comment pointing out that the expressions above are equivalent, we just don't cover this  case at the moment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136173/new/

https://reviews.llvm.org/D136173