Page MenuHomePhabricator

[DebugInstrRef][2/3] Track PHI values through register coalescing
ClosedPublic

Authored by jmorse on Aug 28 2020, 1:26 PM.

Details

Summary

This patch is much like patch 1 in this series -- we maintain a collection of where PHI instructions were and their registers, then update that during coalescing. For background on the largest hunk here: CP contains details about a COPY instruction that is being coalesced, where the "Src" register will be merged into the "Dst" register. Src is also referred to as the "RHS", and Dst as "LHS".

This code will quit tracking PHI values if any very complicated subregister coalesces occur -- but I don't believe register coalescing tries that. (I can come up with proof if needs be).

~

Unrelated to this change, but on the topic of register coalescing, I'd like point out that with DBG_VALUEs, register coalescing is currently breaking variable locations in ways that can only be fixed by instruction referencing. Here's some IR extracted from an LLVM function:

https://gist.github.com/jmorse/4b80df44d5f96ab9ed917d10f625128e

(I don't know how to strip unused metadata,)

And here's the corresponding MIR before PHI Elimination, coalescing, and then after coalescing:

https://gist.github.com/jmorse/3d9972e91cdf94b093f14501a4d14829
https://gist.github.com/jmorse/c45d7f9d5e328018ca884fda6bbde15c
https://gist.github.com/jmorse/bbab447ef6e8f9f596fbceabf188cf1e

You can see in the first gist that the DBG_VALUE refers to a PHI; but in the last, it refers to the ADD64ri8, which is wrong. The reason why is that the DBG_VALUE is out of liveness; but its register gets merged with another VReg, resurrecting it with the wrong value. I tried to fix this ages ago with checkMergingChangesDbgValues, but many variable locations get legitimately merged in this way too, so it couldn't drop all of them.

The only way to fix DBG_VALUEs in this pass would be by using control flow information to work out if a coalesces effect on a DBG_VALUE was legitmate or not; or by tracking the value number that the DBG_VALUE originally referred to. Which is effectively what all this instruction referencing work does.

Diff Detail

Event Timeline

jmorse created this revision.Aug 28 2020, 1:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 1:26 PM
jmorse edited the summary of this revision. (Show Details)Sep 10 2020, 8:57 AM

ping here; I can see the tests here can be improved by removing attributes and a large amount of un-necessary MIR headers, otherwise IMO it's ready to go

aprantl added inline comments.
llvm/lib/CodeGen/RegisterCoalescer.cpp
138

struct PHIValPos ?

139

Either

/// Slot where this PHI occurs.
SlotIndex SI;

or

SlotIndex SI;    ///< Slot where this PHI occurs.
145

Do we need the sorted-ness property of the std::map or should this be a DenseMap?

3618

RegToPHIIdx.insert({CP.getDstReg(), InstrNums});

llvm/test/DebugInfo/MIR/InstrRef/phi-coalesce-subreg.mir
67

are all those attributes necessary?
Nevermind, you already mentioned that in the description...

102

maybe remove all unneeded DILocations from the test?

jmorse updated this revision to Diff 348974.Jun 1 2021, 7:38 AM

Address feedback, delete various un-needed MIR bits and pieces, un-necessary DILocations.

jmorse marked 6 inline comments as done.Jun 1 2021, 7:38 AM
djtodoro accepted this revision.Jun 2 2021, 5:12 AM
djtodoro added a subscriber: djtodoro.

lgtm

This revision is now accepted and ready to land.Jun 2 2021, 5:12 AM

@jmorse @aprantl Sorry for the divergence, but regarding this tests reduction in terms of DI Metadata -- do you think that we can automate this process? I've written a small write-up/proposal for a utility for that purpose: https://github.com/djolertrk/llvm-metadataburn -- please let me know what you think.

jmorse added a comment.Jun 2 2021, 9:32 AM

lgtm

Cheers,

@jmorse @aprantl Sorry for the divergence, but regarding this tests reduction in terms of DI Metadata -- do you think that we can automate this process? I've written a small write-up/proposal for a utility for that purpose: https://github.com/djolertrk/llvm-metadataburn -- please let me know what you think.

Highly relevant to my interests, this'd save 5-10 minutes frequently fiddling with metadata, and it eases newcomers preparation / revision of patches. Yes please!

This revision was landed with ongoing or failed builds.Jun 3 2021, 9:07 AM
This revision was automatically updated to reflect the committed changes.

lgtm

Cheers,

@jmorse @aprantl Sorry for the divergence, but regarding this tests reduction in terms of DI Metadata -- do you think that we can automate this process? I've written a small write-up/proposal for a utility for that purpose: https://github.com/djolertrk/llvm-metadataburn -- please let me know what you think.

Highly relevant to my interests, this'd save 5-10 minutes frequently fiddling with metadata, and it eases newcomers preparation / revision of patches. Yes please!

Yes! Bugpoint can already reduce metadata — I'm wondering if this could maybe a mode of bugpoint in the future?

jmorse added a subscriber: n-omer.Jun 4 2021, 2:26 AM

Yes! Bugpoint can already reduce metadata — I'm wondering if this could maybe a mode of bugpoint in the future?

On this topic, @n-omer will be doing something similar with llvm-reduce (starting imminently), largely inspired by the fact that llvm-reduce can cut an IR file down to one or two blocks and instructions, but leaves 20,000 DI metadata nodes untouched.

(Maybe bugpoint and llvm-reduce are the same thing now? Either day, delta reduction is good).

lgtm

Cheers,

@jmorse @aprantl Sorry for the divergence, but regarding this tests reduction in terms of DI Metadata -- do you think that we can automate this process? I've written a small write-up/proposal for a utility for that purpose: https://github.com/djolertrk/llvm-metadataburn -- please let me know what you think.

Highly relevant to my interests, this'd save 5-10 minutes frequently fiddling with metadata, and it eases newcomers preparation / revision of patches. Yes please!

Yes! Bugpoint can already reduce metadata — I'm wondering if this could maybe a mode of bugpoint in the future?

I guess it can be done in both bugpoint and llvm-reduce -- I am just not sure that by doing this tricks we are preserving "the code equivalence" (since we are removing right metadata) and that it is desirable in these tools.

Yes! Bugpoint can already reduce metadata — I'm wondering if this could maybe a mode of bugpoint in the future?

On this topic, @n-omer will be doing something similar with llvm-reduce (starting imminently), largely inspired by the fact that llvm-reduce can cut an IR file down to one or two blocks and instructions, but leaves 20,000 DI metadata nodes untouched.

(Maybe bugpoint and llvm-reduce are the same thing now? Either day, delta reduction is good).

Cool, I'll make an RFC later today, since we can taint this patch thread with this topic a lot :)