[PATCH] D31604: [DebugInfo][X86] Improve X86 Optimize LEAs handling of debug values.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:550
+  int64_t Offset = (IsIndirect ? MI.getOperand(1).getImm() : 0) + AddrDispShift;
+  if (!IsIndirect && (Offset != 0)) {
+    // If required create new DIExpression with DW_OP_stack_value prepended to
----------------
andrewng wrote:
> aprantl wrote:
> > Is AddrDispShift always > 0?
> I believe that AddrDispShift can be either positive or negative. I assume that it won't be zero as that would imply there were two "identical" LEAs.
Okay then I think there should be an `assert(AddrDispShift != 0 && "...")` here to make sure we change the ?: condition should this ever change in the future.


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:559
+      SmallVector<uint64_t, 8> Elements(OrigElements.size() + 1);
+      Elements[0] = dwarf::DW_OP_stack_value;
+      std::copy(OrigElements.begin(), OrigElements.end(),
----------------
Are you sure you want the DW_OP_stack_value at position 0? Shouldn't it go either at the end of the expression or just before a DW_OP_llvm_fragment operation?


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:561
+      std::copy(OrigElements.begin(), OrigElements.end(),
+                Elements.begin() + 1);
+      Expr = DIExpression::get(Expr->getContext(), Elements);
----------------
I think I would prefer `std::next()` over the `+1`.


https://reviews.llvm.org/D31604