[PATCH] D86814: [DebugInstrRef][3/3] Follow DBG_PHI instructions through LiveDebugValues.

[Tom Weaver via Phabricator via llvm-commits]

TWeaver added a comment.

Looks good to this 'not such an expert in this area' (me).

Code quality looks good minus a couple of nits. I'm unable to make a genuine assessment of the approach to solving this problem due to a lack of experience in this specific area, but it smells like compiler theory to me (thinks hard back to his comp science degree).

As this has been up for so long, and it doesn't seem to be getting much traction, I'd be happy to officially LGTM this, so long as nobody speaks up ASAP.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:3488
+public:
+  SmallVector<std::pair<LDVSSABlock *, uint64_t>, 4> IncomingValues;
+  LDVSSABlock *ParentBlock;
----------------
the uint64_t here seems to denote the number of a value in a preceding LDVSSABlock.

is there anyway we can typedef this away into something more contextual so this pairing reads better? (so long as my assumption about the intent is correct).


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:3490
+  LDVSSABlock *ParentBlock;
+  uint64_t PHIValNum;
+  LDVSSAPhi(uint64_t PHIValNum, LDVSSABlock *ParentBlock)
----------------
same here again, can we do away with the uint64_6 and add a more contextual using/typedefed name instead for readability?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:3745
+  // technically possible for us to merge values in different registers in each
+  // block, but highly unlikely that LLVM will generate such code after register
+  // allocation.
----------------
perhaps this assumption can be asserted?


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

https://reviews.llvm.org/D86814