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

[Andrew Ng via Phabricator via llvm-commits]

andrewng added a comment.

Sorry for the slight delay but only just got back from holiday.



================
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
----------------
aprantl wrote:
> 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.
Could you please explain why the ?: condition would need to change if AddrDispShift == 0. I think it should work as it is and thus there is no need for the assertion.


================
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(),
----------------
aprantl wrote:
> 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?
Yes, I think it needs to be there otherwise any existing operations will be operating on the wrong value.


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


https://reviews.llvm.org/D31604