[PATCH] D143146: [Assignment Tracking] Fix migrateDebuginfo in SROA

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

Orlando added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:168-169
+  // target to fit.
+  if (Target.OffsetInBits < CurrentFragment->OffsetInBits ||
+      Target.SizeInBits > CurrentFragment->SizeInBits)
+    return std::nullopt;
----------------
StephenTozer wrote:
> This looks like it would miss the case where `Target.OffsetInBits + Target.SizeInBits > CurrentFragment->OffsetInBits + CurrentFragment->SizeInBits`, i.e. the target fragment starts inside the current fragment but extends beyond it.
> 
> Optionally, I think the logic in this check might be easier to write and understand if we first set variables `(Target|Current)Slice(Start|End)`, and then both this and the above check could be combined with `TargetSliceStart >= CurrentSliceStart && TargetSliceEnd <= CurrentSliceEnd`. 
Good catch, I've added the helper methods `startInBits` and `endInBits` to `DIExpression::FragmentInfo`.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:187
+///                              is being split.
+/// \param AbsoluteOffsetInBits  New offset relative to \p OldAlloca.
 /// \param SliceSizeInBits       New number of bits being written to.
----------------
StephenTozer wrote:
> Slightly confusing that the name has changed to `AbsoluteOffsetInBits` but the description still says it is relative - is this an update error? If not, I think this might need a variable name that specifies what it is relative to.
Ah you're right that is confusing - is that better? It is a little difficult to convey in a short-ish name exactly what it is + "InBits" (which I think is a useful suffix in the absence of bit/byte types).

To be clear, it's the slice's start bit in OldAlloca rather than its offset in the new alloca (if there is one).


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:254
+
+      if (!CurrentFragment || !(*NewFragment == *CurrentFragment)) {
+        if (CurrentFragment) {
----------------
StephenTozer wrote:
> With the guard against `!NewFragment` above, this could be simplified?
Huh, you're right, nice one. Still have to use the `!(x == y)` form due to lack of `operator!=` overload though.


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

https://reviews.llvm.org/D143146