[PATCH] D133293: [Assignment Tracking][10/*] salvageDebugInfo for dbg.assign intrinsics

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

LGTM with a question mark over whether we can just start off using poison operands for addresses in dbg.assigns (doing it to the value field makes conformance with the existing dbg.value tests harder). Plus the caveat that to avoid regression, dbg.assign will need to support variadic value operands.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1769
+  } else {
+    DAI->setAddress(UndefValue::get(I->getType()));
+  }
----------------
nlopes wrote:
> Orlando wrote:
> > nlopes wrote:
> > > Please use PoisonValue for placeholder/dummy values. We are trying to get rid of undef.
> > > Thank you!
> > Oh right, I wasn't aware of that - do you have a link to a discussion on this?
> > 
> > FWIW, this `Undef` is wrapped in `ValueAsMetadata` and is used by a debug intrinsic, if that makes a difference. This is currently standard practice for debug intrinsics to denote that the value of a variable is unknown.
> > 
> > I posted a question on discourse that might be relevant / you may be interested in - [[ https://discourse.llvm.org/t/auto-undef-debug-uses-of-a-deleted-value/65019 | here ]].
> Traditionally all placeholders were undef, because there was no poison value.
> But now that we have poison, we are trying to move away from undef. Since PoisonValue inherits from UndefValue, all matchers (e.g., isa<>) will work fine. So upgrades can be done incrementally.
> 
> I do have a rule in the review system to alert me when someone is trying to add a new use of UndefValue::get, as we want to avoid that.
@Orlando given that this is a new field that doesn't match existing tests for dbg.value's, will there be much breakage of lit tests if we use Poison here? I think we should prefer to update debug-info use of poison in one fell swoop, but as this is A New Thing (TM) the implications of using poison here should be small, I would imagine.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1799-1801
+    assert((is_contained(DIILocation, &I) || DbgAssignAddrUse) &&
+           "DbgVariableIntrinsic must use salvaged "
+           "instruction as its location");
----------------
Doesn't the rest of this function only apply to the Value field, and for DbgAssigns we've already checked whether the Value field contains the instruction? And if it does contain the instruction, this assertion can't fire, surely, so isn't DbgAssignAddrUse redundant?


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/salvage-value.ll:1
+; RUN: opt %s -S -o - -passes=instcombine | FileCheck %s
+
----------------
I'd suggest an implicit check not for other dbg.assigns


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

https://reviews.llvm.org/D133293