[PATCH] D133307: [Assignment Tracking][14/*] Account for assignment tracking in instcombine

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

Orlando added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:274-278
+    auto *FillVal = ConstantInt::get(ITy, Fill);
+    StoreInst *S = Builder.CreateStore(FillVal, Dest, MI->isVolatile());
+    S->copyMetadata(*MI, LLVMContext::MD_DIAssignID);
+    for (auto *DAI : at::getAssignmentMarkers(S))
+      DAI->setValue(FillVal);
----------------
jmorse wrote:
> jmorse wrote:
> > Orlando wrote:
> > > jmorse wrote:
> > > > Is there a risk that, if multiple stores have the same assign ID (which I think is possible?) that other dbg.assign intrinsics down other code paths will have their Values updated?
> > > > 
> > > > This might not actually be an issue, because if there's a call to memset (this context) then those parts won't have been promoted to Values.
> > > >  This might not actually be an issue, because if there's a call to memset (this context) then those parts won't have been promoted to Values.
> > > 
> > > This matches my feelings too. Except! I tried to get a reproducer for your question in D133294 and actually stumbled into a case where this is... interesting. There's a dbg.assign that is linked to a memset in this situation has an `undef` that gets converted to a non-undef value here.
> > > 
> > > In my reproducer, the `undef` is added as a result of the old behaviour in D133315, which has since changed. But it is conceivable that an `undef` could've been introduced by different circumstances.
> > > 
> > > It feels unnatural for a debug intrinsic to ever leave the "undef" state, but in this case it's definitely not wrong. I don't know if that changes your feelings at all, but thought it was worth mentioning.
> > > 
> > > pre-submit EDIT:
> > > Your concerns (mainline comments) sound possible to me - I would prefer to guard this, I'll update it.
> > ^
> I think recovering 'undef' back to a real Value is on-theme for assignment tracking -- after all, a primary outcome of all of this is that we fall back to using the stack location to identify the variables value, instead of an SSA Value. I suppose a failure mode would be where a store is rewritten to store a different value to what the original assignment "meant": imagine if we had:
> 
>   store %foo, %ptr, !DIAssignID !1
>   store %bar, %ptr, !DIAssignID !2
> 
> And we deleted the second store but mutated the first to store %bar -- then we would restore connected dbg.assigns to the wrong value, if they became undef. But that would be erroneous behaviour anyway (to mutate stores like that), right? Otherwise, it doesn't matter if we restore Values to undef dbg.assigns at arbitary points in the function, so long as the store continues to be a source of truth about the original assignment.
> 
> I'd suggest adding a test for the undef-to-not-undef behaviour for referencing, might be worth putting it in the discourse post too.
Interesting question. I'm not sure if such a transformation exists (as in "I don't know", not "I think not"). In that scenario you are right that the value would become wrong there. The whole "web of shared DIAssignID after unrolling" thing could also be a problem here. It would be nice if we didn't need that, but IIRC we decided it was necessary & I'm not particularly keen on re-evaluating that at this moment.

I've gone with the safest option here (`if (uses old value) use new value`), is that okay with you?

FWIW the test `memset.ll` in this patch was testing the undef -> known value behaviour, though I've now updated it.


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

https://reviews.llvm.org/D133307