This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef] Use PHI placement utilities for variable-value calculations
ClosedPublic

Authored by jmorse on Sep 28 2021, 7:50 AM.

Details

Summary

This patch is very similar to D110173, but for variable values rather than machine values. This is for the second instr-ref problem, calculating the correct variable value on entry to each block. As in D110173, rather than using Jeremys incorrect lattice solution, we use the existing LLVM tooling to place PHI values at control flow merges, and then use value propagation to eliminate un-necessary ones. The only real difference with the machine-value implementation is that we must pick a location for variable PHI values, i.e. when the variable has two different values in predecessors to the block, try to find a register where those same values merge together. That's implemented in pickVPHILoc. Again, I've written a bunch of unit tests to demonstrate and stimulate what different parts of this implementation should be doing.

Into the specifics: I've moved the VLocTracker class into the InstrRefBasedImpl header, so that we can test with it. I's just a collection containing the variable-value transfer function for each block. The DbgValue class, storing the "current value "of a variable while solving, grows a "VPHI" kind to represent variable-value PHIs that haven't been eliminated (yet).

Quite a lot of vlocJoin gets deleted: we no longer try to pick the right variable value and whether there should be a PHI in this function. Instead, PHIs are pre-placed with the LLVM IDF calculator, and vlocJoin just tries to eliminate them. Picking a location for a PHI is handled immediately afterwards by calling pickVPHILoc. The transfer function evaluation doesn't change at all.

In the unit-tests, I've re-factored creation of the block layouts into their own function so that they can be shared, and re-numbered the MachineBasicBlock pointer variables to be zero based, to avoid future confusion. As with the machine-location testing, I can't guarantee the unit tests achieve wonderful coverage, but they're a lot better than nothing and should demonstrate what's supposed to be happening in various scenarios.

The regression test added used to cause an infinite loop in the old lattice arrangement, because it would whip back and forth between trying to have a PHI in $rdx then $rcx, and back again. As I'd already figured out this was basically trying to do SSA construction, it probably could have been fixed... but better to refresh everything in terms that can actually be understood.

Diff Detail

Event Timeline

jmorse created this revision.Sep 28 2021, 7:50 AM
jmorse requested review of this revision.Sep 28 2021, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2021, 7:50 AM
Orlando added inline comments.Sep 28 2021, 10:07 AM
llvm/unittests/CodeGen/InstrRefLDVTest.cpp
507

Over in D110173 I said:

I think that the tests, especially this one, would be easier to read if there was a comment mapping the block numbers to the block names used in the CFG comment at the start of each test. Or, slightly more radically, replace the integer literals with a name. E.g.

const uint64_t Entry = 0, Loop1 = ...;
LiveInRsp(Entry, 0, RspLoc);

I think this is even more desirable now with the block-layout-function refactor.

First pass look through looks fine to me, some minor nits inline but the overall function seems correct.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
1983
2151

Unused variable?

jmorse updated this revision to Diff 378163.Oct 8 2021, 4:24 AM

Update for review comments, labelled all the block numbers with names, adjusted comments, and deleted some now unused variables.

I've also switched the "BlockNo" field of class DbgValue to being an int rather than unsigned: MachineBasicBlock::getNumber returns a signed int, so it makes sense to store something as close to that as possible.

This revision is now accepted and ready to land.Oct 13 2021, 8:16 AM
This revision was landed with ongoing or failed builds.Oct 14 2021, 6:44 AM
This revision was automatically updated to reflect the committed changes.