[PATCH] D133296: [Assignment Tracking][13/*] Account for assignment tracking in SROA

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

As far as I understand it, all the code above `splitAlloca` is about splitting some alloca that already has dbg.assigns, and creating the appropriate dbg.assigns with an updated fragment. Isn't the additions to `splitAlloca` doing exactly the same thing? My feeling is that this is something to do with splitting stores being distinct in some way from splitting based on uses?

Note to self: haven't looked at the tests.

The store splitting stuff is tricky because it's all new behaviours -- I don't think that code has been instrumented for debug-info before, meaning we won't find omissions by tests failing. This is unfortunate, but I think it's is necessary for progress to be made.



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:120-121
 namespace {
+/// FIXME: Is this assumption ok? I think other place in llvm assume an 8
+/// it byte.
+constexpr uint64_t ByteWidth = 8;
----------------
Things like IRBuilderBase::CreateExtractInteger assume an 8 bit byte, so I think we're probably fine to assume it here too.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:132-134
+/// Find linked dbg.assign and generate a new one with the correct
+/// FragmentInfo. Link Inst to the new dbg.assign.  If Value is nullptr the
+/// value component is copied from the old dbg.assign to the new.
----------------
Docu-commenting the parameters would be nice -- I'm guessing they're all parts / components of a store, but it's better to be expilcit.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:178
+      return AllocaSize;
+    }();
+    bool MakeNewFragment = CurrentFragSize != SliceSize;
----------------
Style preference: separate line to call this lambda on.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:189
+      else
+        assert(false && "Failed to create fragment expr!");
+    }
----------------
`llvm_unreachable` is probably more cannonical. Or, assigning the Optional E, asserting it, then dereferencing it. Seems like un-necessary control flow otherwise.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:203
+
+    // We could use more precision here at the const of some additional (code)
+    // complexity - if the original dbg.assign was adjacent to its store, we
----------------



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:214
+    // noted as slightly offset (in code) from the store. In practice this
+    // should have little effect on the dbeugging experience due to the fact
+    // that all the split stores should get the same line number.
----------------



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:215
+    // should have little effect on the dbeugging experience due to the fact
+    // that all the split stores should get the same line number.
+    NewAssign->moveBefore(DbgAssign);
----------------
We should be able to identify difficulties / complications from this in our end-to-end Dexter tests, so I'm not too concerned.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:2700-2703
+    // Capture V now because if it's a scalar it'll get rewritten so that the
+    // store is a vector store. We still want to know the original value being
+    // stored for debug info since we can't index a vector value in a
+    // DIExpression.
----------------
IMO too verbose -- "Capture V for the purpose of debug-info accounting once it's converted to a vector store".


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

https://reviews.llvm.org/D133296