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

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

Orlando added a comment.

Sorry - I didn't submit my inline responses (requires an extra button click after updating the patch, and I forgot this time).



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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:168-169
   New->setUsedWithInAlloca(AI.isUsedWithInAlloca());
+  New->setMetadata(LLVMContext::MD_DIAssignID,
+                   AI.getMetadata(LLVMContext::MD_DIAssignID));
 
----------------
jmorse wrote:
> Note that D133118 fiddles with this area, as the call below to replaceInstUsesWith doesn't update debug users. With D133118 landed, I think this code is fine.
I'll double-check it looks right when rebasing before landing? I think it'd be okay either way (see related reply in test `alloca-bitcast.ll`).


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3942-3945
+    // Leave dbg.assign intrinsics in their original positions and there should
+    // be no need to insert a clone.
+    if (isa<DbgAssignIntrinsic>(User))
+      continue;
----------------
jmorse wrote:
> Colour me delighted.
(this of course relies on the use-before-def being preserved, which probably is a bit hopeful)


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/alloca-bitcast.ll:21
+; CHECK-NEXT: %tmpcast = bitcast i64* %retval to %struct.c*
+; CHECK-NEXT: call void @llvm.dbg.assign(metadata i1 undef, metadata ![[e:[0-9]+]], metadata !DIExpression(), metadata ![[ID]], metadata %struct.c* %tmpcast, metadata !DIExpression()), !dbg
+; CHECK: ![[e]] = !DILocalVariable(name: "e",
----------------
jmorse wrote:
> This then leads to the dbg.assign referring to a cast (`tmpcast`?), not to the alloca itself, which seems wrong. Possibly related to what I mentioned above.
> 
> (non-edit: Or, is it OK?)
I think it's okay either way. We still have the link to the alloca through the `DIAssignID` and all assignment tracking code that looks at addresses to find base-storage will be stripping casts etc. In addition (I mention in the TODO list in the docs) I'm think that the address and address-expression are possibly redundant.


================
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
----------------
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).


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

https://reviews.llvm.org/D133307