[PATCH] D74985: [LiveDebugValues] Encode a location in VarLoc IDs, NFC [2/3]

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 2 inline comments as done.
vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:118
+/// for insertion into a \ref VarLocSet, and efficiently converted back. The
+/// type-checker helps ensure that the conversions aren't lossy.
+///
----------------
aprantl wrote:
> Should we write this as an adaptor/wrapper of SparseBitVector instead?
> I'm fine with "no" as an answer.
I'm not sure I follow, sorry. Do you mean, should we be writing `for (LocIndex LI : someVarLocSet)`, where someVarLocSet is a wrapper around a bitvector that allows iteration over LocIndex? I'm inclined to say 'no', as working with the wrapper to set operations and the like might be messier than converting to/from LocIndex.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:133
+  uint64_t getAsRawInteger() const {
+    return (static_cast<uint64_t>(Location) << 32) | Index;
+  }
----------------
aprantl wrote:
> Out of curiosity: This data structure looks like a hand-written implementation of `union { uint64_t ; struct {int32_t; int32_t}}` are there some UB-related problems with doing just that?
My intention in writing out the left shift explicitly is to make it clear to readers that "Location" lives in the high bits. This isn't clear to me when this is written as a union. There could be weird effects with endianness as Paul points out.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:436
 
-  using VarLocMap = UniqueVector<VarLoc>;
+  class VarLocMap {
+    UniqueVector<VarLoc> VarLocs;
----------------
aprantl wrote:
> If it's has methods, it gets a doxygen comment :-)
Ouch, I accidentally squashed the change to add doxygen comments to VarLocMap into D74986. Is it all right if I land the change there? Splitting the commit apart is a pain.


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

https://reviews.llvm.org/D74985