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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

In D136173#3874232 <https://reviews.llvm.org/D136173#3874232>, @Orlando wrote:

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

There's some justification to use DIExpressions in this function; namely that the `DIExpression::append `logic is slightly non-trivial, and would be a pain if errors started emerging because the append logic gets changed in one place later on - but with that said, I think the performance benefits are justifiable (or perhaps the append logic could be partially extracted to a new function).. Also w.r.t. point 2, that change is necessary as long as we construct the DIExpressions as part of this function (because it will at some point be used to compare entry value expressions, so it will automatically create `DW_OP_LLVM_arg, 0, DW_OP_entry_value, 1` expressions), but can be taken out if we just work with the lists.



================
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,
----------------
Orlando wrote:
> Since this function doesn't take into account the value or DIArgList contents I think it should be called `isEqualExpression` or something. Wdyt?
Reasonable, when I first wrote the function it did compare values, but now that it doesn't a rename is justified.


================
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
----------------
Orlando wrote:
> What happens to the dbg.value above?
All 3 of these dbg.values are given separate entries in the output loclist without this change.


================
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);
----------------
Orlando wrote:
> Maybe worth adding a comment pointing out that the expressions above are equivalent, we just don't cover this  case at the moment?
SGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136173