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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse 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:
> 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.


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/sink.ll:79-80
+f.exit:                                           ; preds = %entry
+  store i8* null, i8** %b, align 8, !dbg !49, !tbaa !50, !DIAssignID !53
+  call void @llvm.dbg.assign(metadata i8* null, metadata !39, metadata !DIExpression(), metadata !53, metadata i8** %b, metadata !DIExpression()), !dbg !41
+  call void (%struct.a*, ...) bitcast (void (...)* @g to void (%struct.a*, ...)*)(%struct.a* nonnull %i) #5, !dbg !54
----------------
Orlando wrote:
> jmorse wrote:
> > If I'm reading the CHECK lines right, it's the definition of %b that sinks, not the store or dbg.assign. That's not quite what the test comment says it covers, sinking of an instruction with a DIAssignID?
> You are right but I think the comment does cover this. The "instruction used by a dbg.assign" is `%b = getelementptr ...` (used-by, rather than linked-to).
Woot, I misread it.


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

https://reviews.llvm.org/D133307