This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking] Coalesce dbg loc definitions with contiguous fragments
ClosedPublic

Authored by Orlando on Mar 27 2023, 9:11 AM.

Details

Summary

MemLocFragmentFill uses an IntervalMap to track which bits of each variable are stack-homed. Intervals with the same value (same stack location base address) are automatically coalesced by the map. This patch changes the analysis to take advantage of that and insert a new dbg loc after each def if any coalescing took place. This results in some additional redundant defs (we insert a def, then another that by definition shadows the previous one if any coalescing took place) but they're all cleaned up thanks to the previous patch in this stack.

This reduces the total number of fragments created by AssignmentTrackingAnalysis which reduces compile time because LiveDebugValues computes SSA for every fragment it encounters. There's a geomean reduction in instructions retired in a CTMark LTO-O3-g build of 0.3% with these two patches.

One small caveat is that this technique can produce partially overlapping fragments (e.g. slice [0, 32) and slice [16, 64)), which we know LiveDebugVariables doesn't really handle correctly. Used in combination with instruction-referencing this isn't a problem, since LiveDebugVariables is effectively side-stepped in instruction-referencing mode.

Diff Detail

Event Timeline

Orlando created this revision.Mar 27 2023, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 9:11 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Mar 27 2023, 9:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 9:11 AM

Good saving; in the future we might find different ways of representing variable fragments and communicating them to LiveDebugValues, or make LDV smarter, but it's good to get this benefit now. I think the more structured fragment model @scott.linder proposed would deliver the sort of guarantees we'd need.

As discussed offline, I think the only thing missing is making this behaviour conditional on instr-ref variable locations being used, to avoid LiveDebugVariables tripping over it. Rather than seeing it as "assignment tracking needs instruction referencing" and jamming it in the frontnend, we can consider it as being "instruction referencing needs this hack to deal with assignment tracking efficiently". That allows other backends to benefit from the work.

llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
612–616

I wonder if there's a pathological case where we effectively store a history of defs that we then have to manually clear up, at reasonable expense.

Not really a criticism, everything in compilers has a pathological case, and this is clearly a net positive for compilation overall.

711–715

In this scenario, would I be right in thinking that the code below shortens "i", inserts "f" in the middle, and then IntervalMap will coalesce them back together if they're the same? (Seems like the right behaviour).

Orlando updated this revision to Diff 509256.Mar 29 2023, 2:04 AM

+ Check if instr-ref is turned on before coalescing fragments (see true table in coalesce-options.ll)
+ Add test coalesce-options.ll

Note that the check uses debuginfoShouldUseDebugInstrRef which misses out the -O0 and ((optnone)) checks that the MachineFunction variation use. Assignment tracking is currently disabled for -O0 (note to self: we should check ((optnone)) in AssignmentTrackingPass too) but either way locations that require coalescing shouldn't be created if there are no optimisations - so no harm should come of this slightly permissive interpretation of instruction-reference enabled-ness.

Orlando added inline comments.Mar 29 2023, 2:10 AM
llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
612–616

I think it would be possible to be smarter about when to insert certain defs or not, but this solution seemed to yield decent results with fairly trivial changes. It could be an area to look at in future rounds of performance improvements.

711–715

Yes exactly that.

note to self: we should check ((optnone)) in AssignmentTrackingPass too

done D147129

jmorse accepted this revision.Mar 29 2023, 3:23 AM

LGTM

This revision is now accepted and ready to land.Mar 29 2023, 3:23 AM
This revision was landed with ongoing or failed builds.Mar 29 2023, 8:00 AM
This revision was automatically updated to reflect the committed changes.