Page MenuHomePhabricator

[WIP] improve debug-info in stack-slot-coloring
Needs ReviewPublic

Authored by markus on Apr 22 2021, 6:56 AM.

Details

Diff Detail

Event Timeline

markus created this revision.Apr 22 2021, 6:56 AM
markus requested review of this revision.Apr 22 2021, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 6:56 AM
markus edited the summary of this revision. (Show Details)Apr 23 2021, 12:14 AM
markus added reviewers: jmorse, dblaikie.
markus updated this revision to Diff 341102.Apr 28 2021, 2:28 AM
markus edited the summary of this revision. (Show Details)

Prototype of the 'terminate debug-info for slots when they go out of liveness, before being merged' idea in the TR.

Looks good as a starting place -- I took this for test drive, see inline comments. On a clang-3.4 build, the "PC ranges covered" statistic ticks down from 66% to 65%, however I believe that's due to rounding. Using llvm-locstats, roughly six thousand variables move out of the 100% covered bucket, or about 0.2% of all variables in clang-3.4. 0% covered goes up by a negligible amount. In my opinion: this is a reasonable sacrifice to make in the name of accuracy, and some of those dropped locations will be incorrect. Plus, there's a longer term plan for not needing this.

The LiveIntervals range only identifies spans of instructions: it doesn't contain any information about control flow. There can be scenarios where a stack slot goes out of liveness but doesn't get terminated by this implementation: for example, when we branch out of of a block, the branch target might be somewhere where the slot isn't live.

Seeing how this is a pragmatic fix to eliminate some false locations, I don't think we need to cover all circumstances with a patch like this -- up to you if you want to cover the extra scenarios.

llvm/lib/CodeGen/StackSlotColoring.cpp
548

I get an assertion about SS being a tombstone value for SS2DbgValues, sometimes it comes out as -2. Elsewhere there are loops that continue/bypass if FI is negative, perhaps that's needed here?

569

Needs to clear the collections you added to the pass

Looks good as a starting place -- I took this for test drive, see inline comments. On a clang-3.4 build, the "PC ranges covered" statistic ticks down from 66% to 65%, however I believe that's due to rounding. Using llvm-locstats, roughly six thousand variables move out of the 100% covered bucket, or about 0.2% of all variables in clang-3.4. 0% covered goes up by a negligible amount. In my opinion: this is a reasonable sacrifice to make in the name of accuracy, and some of those dropped locations will be incorrect. Plus, there's a longer term plan for not needing this.

Doesn't sound too bad then. Thanks for testing!

The LiveIntervals range only identifies spans of instructions: it doesn't contain any information about control flow. There can be scenarios where a stack slot goes out of liveness but doesn't get terminated by this implementation: for example, when we branch out of of a block, the branch target might be somewhere where the slot isn't live.

That is a good point and I actually had not thought about that. I guess it is not too hard to fix though as one can; look for branches leaving these LiveIntervals and terminate at the point of the branch target is that is not also live.

Should be enough right?

Seeing how this is a pragmatic fix to eliminate some false locations, I don't think we need to cover all circumstances with a patch like this -- up to you if you want to cover the extra scenarios.

I will give it a try.

[Digging back through my inbox,]

That is a good point and I actually had not thought about that. I guess it is not too hard to fix though as one can; look for branches leaving these LiveIntervals and terminate at the point of the branch target is that is not also live.

Should be enough right?

Sounds good to me, again there's a small risk that legitimate locations will be terminated when they shouldn't. But, we'll still be able to make an educated judgement about whether it's worth it.

Sounds good to me, again there's a small risk that legitimate locations will be terminated when they shouldn't. But, we'll still be able to make an educated judgement about whether it's worth it.

Yes, may be a bit overconservative but should at least be correct. I gave it a go and then got sidetracked by other tasks. Seems it was not as easy as I originally thought as a SlotIndex does not always map to an instruction (or something like that IIRC). Will try to pick it up again in a not too distant future.