[PATCH] D133926: [DebugInfo] Allow non-stack_value variadic expressions and use in DBG_INSTR_REF

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Generally looks good, I've got a couple of questions inline.

Are there any serious performance implications for, in the direction these patches are moving in, there being lots of non-empty DIExpressions compared to today when they're mostly empty? (I'm thinking we might need a static, pre-allocated one in LLVMContext, if every variable is going to allocate one).



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1140-1145
+  if (Expr->getNumElements() && Expr->isSingleLocationExpression() &&
+      Expr->expr_op_begin()->getOp() == dwarf::DW_OP_LLVM_arg) {
+    SmallVector<uint64_t> Ops(
+        make_range(Expr->elements_begin() + 2, Expr->elements_end()));
+    Expr = DIExpression::get(Expr->getContext(), Ops);
+  }
----------------
Just to check -- this is purely for printing the informative comments on assembly output, it isn't actually functional, yes?


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1503-1511
+    // A DW_OP_stack_value comes at the end, but before a DW_OP_LLVM_fragment.
+    if (StackValue) {
+      if (Op.getOp() == dwarf::DW_OP_stack_value)
+        StackValue = false;
+      else if (Op.getOp() == dwarf::DW_OP_LLVM_fragment) {
+        NewOps.push_back(dwarf::DW_OP_stack_value);
+        StackValue = false;
----------------
Just to confirm, the reason why this wasn't present before was because all variadic expressions had to be stack_value, yes?


================
Comment at: llvm/test/DebugInfo/MIR/InstrRef/dbg-phi-subregister-location.mir:13
 # CHECK-NEXT:  DBG_INSTR_REF {{.+}}, dbg-instr-ref(2, 0)
-# CHECK-NEXT:  DBG_VALUE $dil
+# CHECK-NEXT:  DBG_VALUE_LIST {{.+}} $dil
 --- |
----------------
Does this patch actually make some DBG_VALUEs become DBG_VALUE_LIST? I've missed that part, and see it's happening in various tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133926