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

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

Orlando added a comment.

Apologies for adding an additional round of comments - these should / could have come in the first review.



================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:4145
+          cast<MetadataAsValue>(CI->getArgOperand(2))->getMetadata());
+      Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore, 0);
+      NewCall =
----------------
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.


================
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())
----------------
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.




================
Comment at: llvm/test/Bitcode/upgrade-dbg-addr.ll:18
+; CHECK: declare void @llvm.dbg.value(metadata, metadata, metadata)
+declare void @llvm.dbg.addr(metadata, metadata, metadata)
----------------
jryans wrote:
> Orlando wrote:
> > Did you exclude the metadata definitions on purpose and does the test work like this?
> > 
> > It seems odd to me, but I've not looked at the upgrade tests before.
> The `.ll` content is not that meaningful here because only the `.bc` is actually used as an input (unlike most other tests). Bitcode tests seem to use this pattern regularly from what I could see.
> 
> In any case, yes, the test does pass like this. 🙂 
Oh I see. Ok cool, that makes sense to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144793