Page MenuHomePhabricator

[DebugInstrRef][5/9] Substitute debug value numbers to handle optimisations
AcceptedPublic

Authored by jmorse on Aug 11 2020, 10:11 AM.

Details

Reviewers
aprantl
Group Reviewers
debug-info
Summary

This patch handles two optimisations, TwoAddressInstruction and X86's FixupLEAs pass, both of which optimise by re-creating instructions. For LEAs, various bits of arithmetic are better represented as LEAs on X86, while TwoAddressInstruction sometimes converts instrs into three address instructions if it's profitable. Both of these require substitutions to be created -- test that an entry is made in the MIR output. (Handling of this in LiveDebugValues comes later).

Diff Detail

Event Timeline

jmorse created this revision.Aug 11 2020, 10:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 10:11 AM

Are there other targets that also need to be updated?

Are there other targets that also need to be updated?

Probably all of them; I'm aware of at least one other x86 pass that requires a similar action (x86-fixup-bw-insts) and MachineCSE will need it too. I haven't yet looked at non-x86 targets.

I don't think I wrote this down anywhere else, but I'd like this patch to be seen as a "proof of concept" that debug instruction-numbers can / should be transferred in this way. Comprehensively dealing with all the places this action is required is a large-ish problem, which I'd prefer to address in a different patch series if possible. That means that, were this series to land today, there would be some variable locations dropped; but the work is after all in an experimental state hidden behind a flag.

I wrote:

were this series to land today, there would be some variable locations dropped

To avoid this sounding like there's a huge loss: my current testing with this series and the llvm-locstats script on stage2 clang shows no reduction in the "total [variable] availability" number, and a 2% fall in "PC ranges covered", which I would characterise as "most locations preserved with some corner cases still to address".

jmorse updated this revision to Diff 286518.Aug 19 2020, 2:51 AM

Add another call to substituteDebugValuesForInst that I missed (line 681).

jmorse updated this revision to Diff 286829.Aug 20 2020, 8:40 AM

Rebase for rG60433c63acb7 touching something near this patch, no real change.

aprantl accepted this revision.Aug 20 2020, 9:54 AM
aprantl added a subscriber: vsk.

@vsk We really need a richer API for MBB::erase that takes extra arguments and forces you to think about this.

llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
623

MF->makeDebugValueSubstitution({OldInstrNum, OldIdx}, {NewInstrNum, NewIdx}); ?

This revision is now accepted and ready to land.Aug 20 2020, 9:54 AM