[PATCH] D64595: [Debuginfo][SROA] Need to handle dbg.value in SROA pass.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

In D64595#1635451 <https://reviews.llvm.org/D64595#1635451>, @jmorse wrote:

> Looking good, and this produces the ~3% increase in variable locations in clang-3.4 builds like the previous revisions did too. It looks like the two xfailed tests are testing for the behaviour you're explicitly disabling: it's probably better to just delete them, as this is a deliberate decision to change that behaviour.
>
> Paging @aprantl and @rnk : this patch sounds great to me (avoids dropping struct locations after SROA of inlined functions, increases variable location coverage) by not lower dbg.declare of structs, which IMHO the comment in LowerDbgDeclare indicates wasn't supposed to happen anyway. However, this plays into the wider "escaped variables in memory" problem of PR34136... would either of you feel strongly that this shouldn't happen?


Stepping back a bit: Is the result of SROA multiple smaller allocas?

If SROA is only creating new allocas, then describing them with dbg.declares should do no harm, since a later call to LowerDbgDeclare would truncate their range by inserting new dbg.values at every load. But if SROA is inserting the load, we do need to make sure that the loaded value is tracked by a dbg.value (potentially in addition to tracking the alloca with a dbg.declare). So I guess my question is, where are loads for the SROAed allocas generated?



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3163-3164
     if (DII->getParent() == SrcBlock) {
-      // dbg.value is in the same basic block as the sunk inst, see if we can
-      // salvage it. Clone a new copy of the instruction: on success we need
-      // both salvaged and unsalvaged copies.
-      SmallVector<DbgVariableIntrinsic *, 1> TmpUser{
-          cast<DbgVariableIntrinsic>(DII->clone())};
-
-      if (!salvageDebugInfoForDbgValues(*I, TmpUser)) {
-        // We are unable to salvage: sink the cloned dbg.value, and mark the
-        // original as undef, terminating any earlier variable location.
-        LLVM_DEBUG(dbgs() << "SINK: " << *DII << '\n');
-        TmpUser[0]->insertBefore(&*InsertPos);
-        Value *Undef = UndefValue::get(I->getType());
-        DII->setOperand(0, MetadataAsValue::get(DII->getContext(),
-                                                ValueAsMetadata::get(Undef)));
+      if (DII->getIntrinsicID() == Intrinsic::dbg_declare &&
+          !isa<AllocaInst>(I)) {
+        // There should be only one dbg.declare instruction. Thus we could not
----------------
jmorse wrote:
> Interesting -- are there circumstances where this happens (dbg.declare of something that isn't an alloca)? I suspect it would be IR that didn't make sense, we might be better off disallowing it in the IR Verifier instead.
I must admit I'm confused how this patch relates to the example in the review description. What is the instruction coming in here in the example?


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

https://reviews.llvm.org/D64595