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

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1769
+  } else {
+    DAI->setAddress(UndefValue::get(I->getType()));
+  }
----------------
jmorse wrote:
> 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.
I see - it definitely makes sense to move debug intrinsics away from using Undef then.

While I would personally prefer that all debug intrinsics are updated at once, so that we have a uniform representation in IR, @jmorse is right that the problem surface area does shrink if we restrict the update to just this field as it is not shared by existing debug intrinsics.

However, I am still worried about the additional risk of potential complications given that the patch stack is already quite large, and I'm probably going to have my hands full dealing with the fallout for some time as it is.

@nlopes, would you be satisfied if removing this use of Undef came as a future patch and is put on this project's TODO-list (see D132220)?


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1799-1801
+    assert((is_contained(DIILocation, &I) || DbgAssignAddrUse) &&
+           "DbgVariableIntrinsic must use salvaged "
+           "instruction as its location");
----------------
jmorse wrote:
> 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?
That looks right.  This was probably the result of refactoring things at some point.


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

https://reviews.llvm.org/D133293