[PATCH] D120168: [Debuginfo][LSR} Add support for salvaging variadic dbg.value intrinsics [1/2]

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

Orlando added a comment.

Can this be labelled NFC or is there a functional change here that I've missed?



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6062
 
+  /// Create an offset expression for a given value (usually the IV).
+  void CreateOffsetExpr(int64_t Offset, Value *OffsetValue) {
----------------
Should this be `...from a given value...` rather than `...for a given value...`?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6063
+  /// Create an offset expression for a given value (usually the IV).
+  void CreateOffsetExpr(int64_t Offset, Value *OffsetValue) {
+    pushValue(OffsetValue);
----------------
nit: I think these function names should all start with lower case.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6071
+  /// recovers a location's value.
+  void CreateIterCountExpr(const SCEV *S,
+                           const SCEVDbgValueBuilder &IterationCount,
----------------
nit: the function names in this file appear to have a mix of capitalization style -- since it's already inconsistent it might be better to follow the standard (camelCase)? Not a strong opinion, feel free to ignore.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6147-6148
+  // {DW_OP_LLVM_arg,0}.
+  if (DVIRec.Expr->getNumElements() == 0 && NewExpr.size() > 2)
+    NewExpr.push_back(dwarf::DW_OP_stack_value);
 
----------------
Please can you explain why this is necessary (on phab or in the comment or both)?


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

https://reviews.llvm.org/D120168