[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()));
+  }
----------------
nlopes wrote:
> Orlando wrote:
> > 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)?
> > @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)?
> 
> Sure, as long as it doesn't go to the part of the todo list that is never done :)
> We are serious about removing undef from LLVM, so we would appreciate help in not adding more users of undef long term. Ofc if it's just something temporary, it's perfectly fine.
> Thank you!
Of course. Thank you @nlopes I appreciate that! Added to the TODO list in the docs in D132220.


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

https://reviews.llvm.org/D133293