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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added a comment.

Looks broadly good to me, with some small suggestions.



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


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


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:254
+
+      if (!CurrentFragment || !(*NewFragment == *CurrentFragment)) {
+        if (CurrentFragment) {
----------------
With the guard against `!NewFragment` above, this could be simplified?


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

https://reviews.llvm.org/D143146