[PATCH] D144793: [DebugInfo] Upgrade `dbg.addr` to `dbg.value`

[J. Ryan Stinnett via Phabricator via llvm-commits]

jryans marked an inline comment as done.
jryans added inline comments.


================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:4145
+          cast<MetadataAsValue>(CI->getArgOperand(2))->getMetadata());
+      Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore, 0);
+      NewCall =
----------------
Orlando wrote:
> Actually, thinking about it, shouldn't the `DW_OP_deref` be appended rather than prepended? Before converting the dbg.addr to a dbg.value, the expression modified the address of the variable. In order for that to remain true the `deref` needs to be added to the end of the expression.
Ah yes, good catch! Not sure why I convinced myself "prepend" was the thing to do... We want the fully transformed address from the `dbg.addr` expression, so the `deref` should go at the end.

I'll update this and improve the test coverage to match.


================
Comment at: llvm/test/Bitcode/upgrade-dbg-addr.ll:11
+  ; CHECK-NOT: call void @llvm.dbg.addr
+  ; CHECK: call void @llvm.dbg.value(metadata ptr %num.addr, metadata !16, metadata !DIExpression(DW_OP_deref))
+  call void @llvm.dbg.addr(metadata ptr %num.addr, metadata !16, metadata !DIExpression())
----------------
Orlando wrote:
> jryans wrote:
> > Orlando wrote:
> > > Better to avoid hard-coding metadata in the CHECK directives (I've not used the `[[#]]` syntax before to match numbers but iiuc it should work here).
> > > 
> > Ah, good tip, done!
> It just occurred to me that it might be beneficial to include an existing expression (e.g.`DW_OP_plus, DW_OP_constu, 0` or something) to test the pre/appended-ness of it.
> 
> 
Right, good idea, I will add that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144793