[PATCH] D148536: [Assignment Tracking] Fix fragment error for some DSE-shortened stores

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

I guess we're finding all the twisty turney passages (all different) through the compiler that generate uncertainty over variable locations. On the one hand this is exactly what we want to be finding with assignment tracking, on the other hand it sucks that we're now needing to preserve so much detail. Discussed offline -- possibly the correct path is to relax the verifier requirements and just accept some imprecision (but not inaccuracy) in these intrinsics, it depends on whether this is the last corner case.

For this particular patch I think it's hard to track exactly what's going on given that there are now a bunch of different fragments flying around. Refactoring into some utilities in DebugInfoMetadata.cpp would be preferred to reduce the amount we're expanding the DSE file by, plus some ascii-art diagrams of exactly what fragments we're working with and manipulating. It's going to be hard to maintain in the future unless it's very plain. IMO as the terms "variable space" and "dest space" don't show up elsewhere in assignment tracking some more standardised nomenclature should be found.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:547
 
-  auto CreateDeadFragExpr = [Inst, DeadFragment]() {
-    // FIXME: This should be using the DIExpression in the Alloca's dbg.assign
-    // for the variable, since that could also contain a fragment?
-    return *DIExpression::createFragmentExpression(
-        DIExpression::get(Inst->getContext(), std::nullopt),
+  // Move the dead slice from "dest space" into "variable space".
+  int64_t NewOffsetInBits =
----------------
Could you elaborate on what these terms mean here (not clear to me)


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:572-578
+    // createFragmentExpression expects a relative offset if there is already a
+    // fragment.
+    uint64_t RelativeOffset = DeadFragment.OffsetInBits -
+                              DAI->getExpression()
+                                  ->getFragmentInfo()
+                                  .value_or(DIExpression::FragmentInfo(0, 0))
+                                  .OffsetInBits;
----------------
I feel there's not enough (mental) context here to work out what's going on -- relative to what?


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:608-609
+  auto Linked = at::getAssignmentMarkers(Inst);
+  for (auto *DAI :
+       SmallVector<DbgAssignIntrinsic *>(Linked.begin(), Linked.end())) {
+    std::optional<DIExpression::FragmentInfo> NewFragment;
----------------
Just to make me feel better, could you make the SmallVector a named variable please, simply for ease of reading.


================
Comment at: llvm/test/DebugInfo/Generic/assignment-tracking/dse/shorten.ll:45
   call void @llvm.dbg.assign(metadata i1 undef, metadata !11, metadata !DIExpression(), metadata !16, metadata ptr %local, metadata !DIExpression()), !dbg !17
-  call void @llvm.lifetime.start.p0i8(i64 80, ptr nonnull %local) #5, !dbg !18
   %arraydecay = getelementptr inbounds [20 x i32], ptr %local, i64 0, i64 0, !dbg !19
----------------
Reason for deleting these?


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

https://reviews.llvm.org/D148536