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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

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?



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


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

https://reviews.llvm.org/D64595