This is an archive of the discontinued LLVM Phabricator instance.

RegScavenging: Add scavengeRegisterBackwards()
ClosedPublic

Authored by MatzeB on Jun 29 2016, 8:18 PM.

Details

Summary

This is a variant of scavengeRegister() that works for
enterBasicBlockEnd()/backward(). The benefit of the backward mode is
that it is not affected by incomplete kill flags.

This patch also changes
PrologEpilogInserter::doScavengeFrameVirtualRegs() to use the register
scavenger in backwards mode.

This patch is a big step towards being independent of kill flags.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 62334.Jun 29 2016, 8:18 PM
MatzeB retitled this revision from to RegScavenging: Add scavengeRegisterBackwards().
MatzeB updated this object.
MatzeB added reviewers: qcolombet, hfinkel.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
t.p.northover added inline comments.
include/llvm/CodeGen/RegisterScavenging.h
168–169 ↗(On Diff #62334)

Could you be explicit about what the range is here?

I'm guessing To is before this->MBBI and that "inclusive" only applies to both, so the range would be [To, this->MBBI] (i.e. neither To nor this->MBBI may clobber the register).

lib/CodeGen/PrologEpilogInserter.cpp
1169–1171 ↗(On Diff #62334)

I don't follow this 2-address exception. It looks like for something like

1: %v0 = INST %v0, ...
[...]
2: %v0 = INST
[...]
3: INST %v0<kill>

we'd find DefMI is instruction 2 and look for a register that's free between 2 & 3, but then set %v0 to this register everywhere. What happens if that register is defined between 1 & 2?

It seems unlikely that the first Def will be an instruction like 1 (though not impossible and w0, w0, #0 or something could exist), but then again the whole exception is pointless if it can't happen.

MatzeB added inline comments.Jul 6 2016, 2:02 PM
include/llvm/CodeGen/RegisterScavenging.h
168–169 ↗(On Diff #62334)

Yes you guessed correctly; I'll change the wording to be clearer.

lib/CodeGen/PrologEpilogInserter.cpp
1169–1171 ↗(On Diff #62334)

The idea here is that ideally we would only want 1 definition for each virtual register. However this would not work with two-address instructions as something like:

`
%v0 = XXX
%v1 = INST %v0<tied-0>
`
would give you no guarantees that v1 receives the same register as v0. So instead of just allowing a single definition I also allow additional definitions for instructions that read and write the register at the same time.

The example you presented would be invalid in this scheme as the first definition is not allowed: The first definition is not allowed to read the register while subsequent definitions are only allowed if the instruction reads the register at the same time. Also note that this is not a new requirement, this just documents the status-quo.

I can try to reword the comment, the code may be slightly confusing because the MachineRegisterInfo::def_iterator gives no ordering guarantees so I cannot enforce the rules/abort when they are broken.

t.p.northover added inline comments.Jul 6 2016, 2:33 PM
lib/CodeGen/PrologEpilogInserter.cpp
1169–1171 ↗(On Diff #62334)

Ah, I see. So we're actually searching through an annoyingly randomized list for the topmost definition. I understand the comment too now.

So why do we stop the search one before the end (middle clause of for)? Is it an over-zealous attempt to make sure FirstDef is valid on exit?

Without that std::find_if may (or may not, depending on taste) be a little clearer:

auto FirstDef = std::find_if(MRI.def_begin(VReg), MRI.def_end(), [](MachineRegisterInfo::def_iterator I) {
  return !I->getParent()->readsRegister(VReg, TRI);
});
assert(FirstMI != MRI.def_end() && "cannot find first definition");
t.p.northover added inline comments.Jul 6 2016, 2:35 PM
lib/CodeGen/PrologEpilogInserter.cpp
1169–1171 ↗(On Diff #62334)

Ah no, the extra weirdness is harmless: when it changes behaviour we must be on the canonical definition anyway.

MatzeB updated this revision to Diff 63014.Jul 6 2016, 7:18 PM
MatzeB edited edge metadata.

Thanks for the review. I improved the comments and used std:find_if which does indeed improve code clarity.

t.p.northover accepted this revision.Jul 8 2016, 1:34 PM
t.p.northover added a reviewer: t.p.northover.
t.p.northover added inline comments.
test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
3–5 ↗(On Diff #63014)

Don't we need to come up with an alternative that does need a spill slot then? Not that I actually have any idea how to do that on MIPS, but XFAILing the test probably means it'll just be forgotten forever.

This revision is now accepted and ready to land.Jul 8 2016, 1:34 PM
dsanders added inline comments.Jul 11 2016, 2:26 AM
test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
3–5 ↗(On Diff #63014)

I'd prefer not to xfail unless we have to.

Not that I actually have any idea how to do that on MIPS

I think this test is relying on a stack frame that exceeds 16-bits and is using the '%unused_variable = alloca [16384 x i32], align 4' to make sure that happens. I'd guess that the change has made the stack frame smaller somehow and all the offsets fit in a 16-bit integer. Have some of the alloca's been optimized out?

The other causes of an emergency spill slot involve DSP accumulators and double-precision floats but they aren't used in this test.

MatzeB updated this revision to Diff 63538.Jul 11 2016, 10:56 AM
MatzeB edited edge metadata.

Daniel, I reworked test/CodeGen/Mips/emergency-spill-slot-near-fp.ll. Can you take a look if the test looks okay? In it's current form it forces the RegisterScavenger to spill 1 register and I check that it ends up in 0($sp) to make sure it did not end up on the other side of the big "%stackspace" object.

Daniel, I reworked test/CodeGen/Mips/emergency-spill-slot-near-fp.ll. Can you take a look if the test looks okay? In it's current form it forces the RegisterScavenger to spill 1 register and I check that it ends up in 0($sp) to make sure it did not end up on the other side of the big "%stackspace" object.

Daniel: ping!

qcolombet accepted this revision.Jul 18 2016, 5:18 PM
qcolombet edited edge metadata.

LGTM as well.

@Daniel, leaving you the final approval for the MIPS test case.

dsanders accepted this revision.Jul 19 2016, 3:25 AM
dsanders added a reviewer: dsanders.

The MIPS test LGTM, it's a lot clearer about how it's going about the test too. Thanks, and sorry for the delay.

This revision was automatically updated to reflect the committed changes.