[PATCH] D36596: [InstCombine] Don't convert all dbg.declares to dbg.values

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

> I think this patch is an incremental step towards what I sketched out. In the long run, the frontend wouldn't emit dbg.declare, it would emit dbg.value+DW_OP_deref.

That's something I'm not yet sure about. With a dbg.declare it is very clear that the location is not just describing the value of the source variable, but can also be written to by the debugger, when the user wants to assign the variable a new value. The DWARF standard call this implicit vs. explicit location descriptions. We don't really distinguish between these two cases in LLVM yet, but dbg.value vs dbg.declare seems to make sense for it.

That said, this is not something we need to decide on for this patch :-)

Do you think you could factor the code in this patch into llvm::salvageDebugInfo() (found in Local.cpp)? There doesn't seem to be anything specific to InstCombine and this will make it easier to apply it to other passes later.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2061
-          case Intrinsic::dbg_declare:
-          case Intrinsic::dbg_value:
           case Intrinsic::invariant_start:
----------------
rnk wrote:
> aprantl wrote:
> > What's the effect of this change?
> Now that we have `ValueAsMetadata`, these no longer appear in use lists. These case labels are just stale. I'll just commit this separately.
Makes sense, thanks!


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2138
         replaceInstUsesWith(*I, UndefValue::get(I->getType()));
+      } else if (DDI && isa<StoreInst>(I)) {
+        ConvertDebugDeclareToDebugValue(DDI, cast<StoreInst>(I), *DIB);
----------------
I think this got lost in my flurry of other comments...
Don't we need to handle loads (and potentially calls) similarly (see the example in my previous comment)?


https://reviews.llvm.org/D36596