[PATCH] D67217: [Debuginfo][Instcombiner] Do not clone dbg.declare in TryToSinkInstruction()

[Adrian Prantl via Phabricator via llvm-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

This is a strict improvement and as such is fine, but there's some general cleanups that are uncovered by this that we should also be doing.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3173-3175
+        DII->setOperand(
+            0, MetadataAsValue::get(I->getContext(),
+                                    ValueAsMetadata::get(I->getOperand(0))));
----------------
jmorse wrote:
> As I understand it, this works right now because the only sinkable instructions that dbg.declares point at are casts of other things (like arguments). This is probably fine -- but if dbg.declares operand rules ever change or someone writes IR that doesn't match that expectation, getOperand/setOperand could fail and it won't be clear why.
> 
> IMO it'd be good to add something like
> 
>     if (!isa<Cast>(I))
>       continue; // dbg.declare points at something it shouldn't
> 
> so that the failure mode is a debug use-before-def (which I don't believe is an error) rather than a crash.
We could probably simplify the model by enforcing that dbg.declares always point at allocas or function arguments directly, That would make code like this one look less murky.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3169
+      if (isa<DbgDeclareInst>(DII)) {
+        // dbg.declare instruction should not be cloned. It should be left in
+        // the original place since sunk instruction is not an alloca(otherwise we
----------------
How about:

```
A dbg.declare instruction should not be cloned, since there can only be one per variable fragment.
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3170
+        // dbg.declare instruction should not be cloned. It should be left in
+        // the original place since sunk instruction is not an alloca(otherwise we
+        // could not be here). But we need to update arguments of dbg.declare
----------------
space before `(`


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3174
+        if (!isa<CastInst>(I))
+          continue; // dbg.declare points at something it shouldn't
+
----------------
This should really be a Verifier error.


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

https://reviews.llvm.org/D67217