This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Redo analyzePhysRegs() and computeRegisterLiveness()
ClosedPublic

Authored by MatzeB on Dec 7 2015, 6:00 PM.

Details

Summary

computeRegisterLiveness() was broken in that it reported dead for a
register even if a subregister was alive. I assume this was because the
results of analayzePhysRegs() are hard to understand with respect to
subregisters.

This commit: Changes the results of analyzePhysRegs (=struct
PhysRegInfo) to be clearly understandable, also renames the fields to
avoid silent breakage of third-party code (and improve the grammar).

Fix all (two) users of computeRegisterLiveness() in llvm: By reenabling
it and removing workarounds for the bug.

This fixes http://llvm.org/PR24535 and http://llvm.org/PR25033

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 42131.Dec 7 2015, 6:00 PM
MatzeB retitled this revision from to CodeGen: Redo analyzePhysRegs() and computeRegisterLiveness().
MatzeB updated this object.
MatzeB added reviewers: qcolombet, atrick.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added subscribers: llvm-commits, jfb, sanjoy.
sanjoy added a comment.Dec 7 2015, 6:28 PM

Minor nits inline.

include/llvm/CodeGen/MachineInstrBundle.h
169 ↗(On Diff #42131)

Please add a one-liner about what 'clobbers' means.

lib/CodeGen/MachineInstrBundle.cpp
328 ↗(On Diff #42131)

Nit: repeated ; s.

MatzeB updated this revision to Diff 42134.Dec 7 2015, 6:36 PM
MatzeB marked an inline comment as done.

Fix reported nitpicks and some improve some small things I found myself (continue shortcut in a loop, comment formatting)

include/llvm/CodeGen/MachineInstrBundle.h
169 ↗(On Diff #42131)

I hopefully made it more clear what this flag means and referenced MachineOperand::CreateRegMask() for a definition of what the bits in the regmask mean.

jfb added a comment.Dec 8 2015, 12:43 AM

Thanks for looking into this!

Did you re-run the repro programs in the bugs linked here? I want to make sure they all still pass.

include/llvm/CodeGen/MachineInstrBundle.h
173 ↗(On Diff #42134)

Can you change uses of "overlapping" here and elsewhere? I think they're part of the confusion.

lib/Target/X86/X86InstrInfo.cpp
4454 ↗(On Diff #42134)

Could you also fix peephole-na-phys-copy-folding.ll? Or does it trigger the verifier as the new FIXME says? We could pass a bigger Neighborhood value to computeRegisterLiveness if that's the case.

MatzeB added inline comments.Dec 8 2015, 10:46 AM
include/llvm/CodeGen/MachineInstrBundle.h
173 ↗(On Diff #42134)

Hmm, not sure what a better description would be.
What about "Reg or one of its aliases is defined. The definition may only cover parts of the register." and then
"Reg or a super-register is defined. The definition covers the full register." below?

lib/Target/X86/X86InstrInfo.cpp
4454 ↗(On Diff #42134)

Yes peephole-na-phys-copy-folding is affected by the FIXME I added (I'll add the name of the test to the comment). This is still an unsolved issue before and after the changes. Increasing the Neighborhood does not help as we need an "infinite" neighborhood to cover all cases. To get a precise liveness query we would need a LivePhysRegs (or RegisterScavenger) instance which ideally is kept up to date while we walk backwards over the basic blocks unfortunately ExpandPostRAPseudos has no easy way to maintain state and walks forward at the moment, so fixing this properly is work for another patch.

jfb added inline comments.Dec 9 2015, 12:50 AM
include/llvm/CodeGen/MachineInstrBundle.h
173 ↗(On Diff #42134)

Sub-register?

And / or ASCII art comment :-)

lib/Target/X86/X86InstrInfo.cpp
4454 ↗(On Diff #42134)

OK, keeping the comment here would be nice. The test file also has a FIXME which you seem to say can't be fixed while addressing the issue it mentions? Maybe it would be worth pulling out the tests that fail validation into their own test that don't have validation (so the other ones can be validated independently)?

hfinkel added a subscriber: hfinkel.Dec 9 2015, 5:50 AM
hfinkel added inline comments.
lib/Target/X86/X86InstrInfo.cpp
4446 ↗(On Diff #42134)

This seems like an odd FIXME to keep now that we have a way to detect the situation. Why don't you detect the LQR_Unknown here and abort instead of doing something the verifier will complain about? Then we can have a FIXME that we really should be using some real analysis here instead of a local search.

jfb added inline comments.Dec 9 2015, 6:05 AM
lib/Target/X86/X86InstrInfo.cpp
4446 ↗(On Diff #42134)

I'm not sure I understand what you're suggesting, maybe the new FIXME isn't clear enough: you're right that a real analysis would fix the issue, but if the current one says unknown then we have to save/restore AX otherwise we may be clobbering live state. It's better to complain at verification time rather than to clobber state!

hfinkel added inline comments.Dec 9 2015, 6:09 AM
lib/Target/X86/X86InstrInfo.cpp
4446 ↗(On Diff #42134)

Indeed, thanks for clarifying. You're right, we can't do better than this for now.

jfb added inline comments.Dec 9 2015, 6:14 AM
lib/Target/X86/X86InstrInfo.cpp
4446 ↗(On Diff #42134)

OK, it would be good to clarify the FIXME then, if Hal didn't get it then it's not clear enough :)

MatzeB added inline comments.Dec 9 2015, 1:45 PM
include/llvm/CodeGen/MachineInstrBundle.h
173 ↗(On Diff #42134)

I meant that the 2nd quote goes to the 2nd variable like this:

/// Reg or one of its aliases is defined. The definition may only cover
/// parts of the register.
bool Defined;
/// Reg or a super-register is defined. The definition covers the full
/// register.
bool FullyDefined;

/// Reg or ont of its aliases is read. The register may only be read
/// partially.
bool Read;
/// Reg or a super-register is read. The full register is read.
bool FullyRead;

I added the 2nd sentence in all these comments so you do not necessarily have to understand what sub-/super-register means here. To me the comments are perfectly clear now. This is not the place to explain concepts like sub- and superregisters with ASCII art IMO (keep in mind that this is a rarely used heuristic for circumstances where you cannot get the RegisterScavenger/LivePhysRegs working).

lib/Target/X86/X86InstrInfo.cpp
4446 ↗(On Diff #42134)

Again I am not sure where the confusion comes from (obviously I try to write comments that make sense to myself). I am guessing the thing here is the MI representation (or rather the MachineVerifier) requires you to add the "undef" flag when reading from dead registers. We do not (and should not IMO) have a way to express a situation in which we do not know whether a register is dead or not.

Anyway I'll change it to this:

// FIXME: If computeRegisterLiveness() reported LQR_Unknown then AX may
// actually be dead. This is not a problem for correctness as we are just
// (unnecessarily) saving+restoring a dead register. However the
// MachineVerifier expects operands that read from dead registers
// to be marked with the "undef" flag.
4454 ↗(On Diff #42134)

Strange, I looked at peephole-na-phys-copy-folding again and it works with -verify-machineinstrs (there are not emergency push ax generated anymore). I must have tested an intermediate version. So I'll just remove the FIXME and add -verify-machineinstrs to peephole-na-phys-copy-folding.ll

qcolombet accepted this revision.Dec 10 2015, 10:58 AM
qcolombet edited edge metadata.

Hi Matthias,

LGTM.

Thanks,
Q.

This revision is now accepted and ready to land.Dec 10 2015, 10:58 AM
jfb added a comment.Dec 10 2015, 12:13 PM

lgtm after the peephole-na-phys-copy-folding.ll update and the two proposed comment updates. Thanks!

This revision was automatically updated to reflect the committed changes.