[PATCH] D21885: RegScavenging: Add scavengeRegisterBackwards()

[Tim Northover via llvm-commits]

t.p.northover added inline comments.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:1169-1171
@@ +1168,5 @@
+
+  // The first definition writes the register, subsequent definitions are
+  // allowed but only if the instructions read+write the register (typical
+  // for two address code). Search for the first definition.
+  const TargetRegisterInfo *TRI = MRI.getTargetRegisterInfo();
----------------
t.p.northover wrote:
> MatzeB wrote:
> > t.p.northover wrote:
> > > 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.
> > 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.
> 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");
Ah no, the extra weirdness is harmless: when it changes behaviour we must be on the canonical definition anyway.


Repository:
  rL LLVM

http://reviews.llvm.org/D21885