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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2782
+    /// Return the index of the bit after the end of the fragment, e.g. for
+    /// fragment offset=0 size=32, return 32.
+    uint64_t endInBits() const { return OffsetInBits + SizeInBits; }
----------------
Nit so small you'd need an electron microscope (so feel free to ignore if you disagree), but imo if an illustrative example is being used to demonstrate that this returns the sum of the size and offset, they should both be non-zero values to make it clear that it's the sum and not just selecting the size.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:167-168
+  // target to fit.
+  if (Target.startInBits() < CurrentFragment->startInBits() ||
+      Target.endInBits() > CurrentFragment->endInBits())
+    return std::nullopt;
----------------
This check now subsumes the check above - since the fragment offset and size are always positive (we've got bigger problems if they aren't!), `CurrentFragment->endInBits() <= Target.startInBits()` will never be true if `Target.startInBits() < CurrentFragment->startInBits()` isn't (and same for the second condition).


================
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.
----------------
Orlando wrote:
> 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).
YMMV but better to have a long-but-descriptive name than a short name that obscures or confuses the meaning of the variable - new one LGTM!


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

https://reviews.llvm.org/D143146