# [DebugInfo][X86] Improve X86 Optimize LEAs handling of debug values.ClosedPublicActions

Authored by andrewng on Apr 3 2017, 8:35 AM.

# Details

Summary

This is a follow up to the fix in r298360 to improve the handling of debug values when redundant LEAs are removed. The fix in r298360 effectively discarded the debug values. This patch now attempts to preserve the debug values by using the DWARF DW_OP_stack_value operation via prependDIExpr.

Moved functions appendOffset and prependDIExpr from Local.cpp to DebugInfoMetadata.cpp and make them available as static member functions of DIExpression.

# Diff Detail

Repository
rL LLVM

### Event Timeline

andrewng created this revision.Apr 3 2017, 8:35 AM
aprantl added inline comments.Apr 3 2017, 9:08 AM
lib/Target/X86/X86OptimizeLEAs.cpp
267 ↗(On Diff #93871)

No need to spell out \brief. LLVM uses autobrief in the doxygen settings.

544 ↗(On Diff #93871)

this comment should go into the header instead.

555 ↗(On Diff #93871)

This is for the backend to decide and discard. We shouldn't have any DWARF version specific code here. (E.g.: what if we generate CodeView debug info?)

After landing https://reviews.llvm.org/D31439, DwarfExpression can actually drop locations that can not be expressed in the requested DWARF version.

andrewng added inline comments.Apr 3 2017, 10:57 AM
lib/Target/X86/X86OptimizeLEAs.cpp
544 ↗(On Diff #93871)

Do you mean the class definition?

555 ↗(On Diff #93871)

OK, so I should just generate the DIExpression with the DW_OP_stack_value operation and the backend will discard the DBG_VALUE if the DWARF version is not compatible (in the case of DWARF debug info)?

aprantl added inline comments.Apr 3 2017, 11:11 AM
lib/Target/X86/X86OptimizeLEAs.cpp
555 ↗(On Diff #93871)

Yes, please. Note that the backend cannot (yet) doe this, but I will hopefully be able to land this functionality this week.

andrewng updated this revision to Diff 94056.Apr 4 2017, 5:38 AM

Removed DWARF version checks as suggested by Adrian (thanks) and updated comments.

aprantl added inline comments.Apr 20 2017, 9:41 AM
lib/Target/X86/X86OptimizeLEAs.cpp
550 ↗(On Diff #94056)

Is AddrDispShift always > 0?

aprantl added inline comments.Apr 20 2017, 2:02 PM
lib/Target/X86/X86OptimizeLEAs.cpp
555 ↗(On Diff #93871)

This change is finally in r300882.

andrewng added inline comments.Apr 21 2017, 3:02 AM
lib/Target/X86/X86OptimizeLEAs.cpp
550 ↗(On Diff #94056)

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.

aprantl added inline comments.Apr 21 2017, 8:53 AM
lib/Target/X86/X86OptimizeLEAs.cpp
550 ↗(On Diff #94056)

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.

559 ↗(On Diff #94056)

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?

561 ↗(On Diff #94056)

I think I would prefer std::next() over the +1.

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

lib/Target/X86/X86OptimizeLEAs.cpp
550 ↗(On Diff #94056)

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.

559 ↗(On Diff #94056)

Yes, I think it needs to be there otherwise any existing operations will be operating on the wrong value.

561 ↗(On Diff #94056)

OK, I will change this.

aprantl added inline comments.Apr 25 2017, 9:14 AM
lib/Target/X86/X86OptimizeLEAs.cpp
559 ↗(On Diff #94056)

Sorry, this was a rhetorical question. The DW_OP_stack_value operation always terminates the expression. It *must* come at the end (but before DW_OP_LLVM_fragement). You can factor out the code in Local.cpp from prependDIExpr() and move it into a static member function of DIExpression.

andrewng added inline comments.Apr 25 2017, 10:03 AM
lib/Target/X86/X86OptimizeLEAs.cpp
559 ↗(On Diff #94056)

Thanks for the clarification. I re-read the spec more carefully and yes, I missed the "terminates the expression" part.

I've had a quick look at prependDIExpr. Could I use the offset to apply my AddrDispShift? Will the backend then sort out the specifics of generating a "correct" DWARF expression?

I think it is somewhat confusing that the DIExpression uses DWARF operations, but isn't strictly the equivalent of a DWARF expression. Or have I misunderstood?

aprantl added inline comments.Apr 25 2017, 2:55 PM
lib/Target/X86/X86OptimizeLEAs.cpp
559 ↗(On Diff #94056)

I've had a quick look at prependDIExpr. Could I use the offset to apply my AddrDispShift? Will the backend then sort out the specifics of generating a "correct" DWARF expression?

I think so. Can you give me an example of how you imagine it failing, perhaps I'm not understanding your concern correctly?

I think it is somewhat confusing that the DIExpression uses DWARF operations, but isn't strictly the equivalent of a DWARF expression. Or have I misunderstood?

I recently tried to improve the documentation in LangRef.rst let me know if there are still things missing. The most painful difference that I'm aware of right now is the fact that DW_OP_plus/minus has an implicit operand.

andrewng added inline comments.Apr 26 2017, 1:08 PM
lib/Target/X86/X86OptimizeLEAs.cpp
559 ↗(On Diff #94056)

I will experiment with prependDIExpr. It looks like it should do what I need. I'll get back to you if I have any issues.

I've looked at the DIExpression documentation and it looks good to me.

andrewng updated this revision to Diff 96920.Apr 27 2017, 8:03 AM
andrewng edited the summary of this revision. (Show Details)

As suggested by Adrian, I have made prependDIExpr a static member function of DIExpression (and also appendOffset used by prependDIExpr). This is then used to apply the LEA address shift to the replacement debug value.

aprantl accepted this revision.Apr 27 2017, 9:01 AM

Thanks!

This revision is now accepted and ready to land.Apr 27 2017, 9:01 AM
This revision was automatically updated to reflect the committed changes.