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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Some questions, but looks good, and good to have test coverage for all of this.



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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:168-169
   New->setUsedWithInAlloca(AI.isUsedWithInAlloca());
+  New->setMetadata(LLVMContext::MD_DIAssignID,
+                   AI.getMetadata(LLVMContext::MD_DIAssignID));
 
----------------
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.


================
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;
----------------
Colour me delighted.


================
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",
----------------
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?)


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/sink-store.ll:28-29
+
+; CHECK: if.end:
+; CHECK-NEXT: store i32 2, i32* %local{{.*}}!DIAssignID ![[MERGED_ID]]
+
----------------
Non-review: steamingly good that we can represent effective use-before-store / def's like this.


================
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
----------------
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?


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/storemerge.ll:83
+; CHECK-NEXT: %storemerge2 = phi float [ %call2.2, %if.then.2 ], [ %call5.2, %if.else.2 ]
+; CHECK-NETX: store float %storemerge2, float* %ref.tmp.sroa.0.0..sroa_idx, align 4{{.+}}!DIAssignID ![[id]]
+
----------------
NEXT


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

https://reviews.llvm.org/D133307