[PATCH] Modify the register scavenger to operate on reg units when checking liveness

[Jakob Stoklund Olesen]

> On Jul 31, 2014, at 4:17 PM, Pedro Artigas <partigas at apple.com> wrote:
> 
> Here is the new version of the patch that addresses the comments by Quentin and Jacob and removes the code for the callee saved bit vector. Compile time measurements indicate no advantage for the changes to tail duplication and the generation of the used bit vector but no regression on average either.

Hi Pedro,

Comments below.

> Index: include/llvm/CodeGen/MachineRegisterInfo.h
> ===================================================================
> --- include/llvm/CodeGen/MachineRegisterInfo.h	(revision 214448)
> +++ include/llvm/CodeGen/MachineRegisterInfo.h	(working copy)
> @@ -516,6 +516,23 @@
>   /// That function will return NULL if the virtual registers have incompatible
>   /// constraints.
>   void replaceRegWith(unsigned FromReg, unsigned ToReg);
> +  
> +  /// replaceVRegWithPhysReg - Replace all instances of virtual register
> +  /// fromReg with physical register ToReg, also applies sub-registers in
> +  /// operands to map to final physical register.
> +  ///
> +  /// Example:
> +  ///
> +  /// vreg10 =
> +  /// = vreg10.sub0
> +  /// = vreg10.sub1
> +  ///
> +  /// ==>
> +  ///
> +  /// R5_R6 =
> +  /// = R5
> +  /// = R6
> +  void replaceVRegWithPhysReg(unsigned FromReg, unsigned ToReg);

I don’t think a new function is needed here. Just make the existing replaceRegWith do the right thing when ToReg is a physreg. I can’t imagine a situation where you would want to use the existing functionality with a physreg.

Also, in the implementation please use MachineOperand::substPhysReg() instead of duplicating that functionality.

> @@ -112,6 +111,9 @@
>   MachineBasicBlock::iterator getCurrentPosition() const {
>     return MBBI;
>   }
> +  
> +  /// isRegUsed - return if a specific register is currently used.
> +  bool isRegUsed(int Reg, bool includeReserved);
> 

> +  /// isUsed - Test if a register is currently being used. Check reserved
> +  /// registers also if requested.
> +  bool isUsed(unsigned Reg, bool CheckReserved = true) const;

What is the difference between isRegUsed() and isUsed()? Why does one accept a signed Reg argument? Do we need both?

> +  /// setUsed / setUnused - Mark the state of one or a number of register units.
>   ///
>   void setUsed(BitVector &Regs) {
> -    RegsAvailable.reset(Regs);
> +    RegUnitsAvailable.reset(Regs);
>   }
>   void setUnused(BitVector &Regs) {
> -    RegsAvailable |= Regs;
> +    RegUnitsAvailable |= Regs;
>   }

If Regs is a vector of regunits, it shouldn’t be called ‘Regs'.

> Index: lib/CodeGen/MachineRegisterInfo.cpp
> ===================================================================
> --- lib/CodeGen/MachineRegisterInfo.cpp	(revision 214448)
> +++ lib/CodeGen/MachineRegisterInfo.cpp	(working copy)
> @@ -294,7 +294,32 @@
>   }
> }
> 
> +/// replaceVRegWithPhysReg - Replace all instances of FromReg with ToReg in the
> +/// machine function. Note that ToReg must be a physical register; that is
> +/// important as we process the sub-registers applied to the operand to generate
> +/// a proper phys reg.
> 
> +void MachineRegisterInfo::replaceVRegWithPhysReg(unsigned FromReg,
> +                                                 unsigned ToReg) {
> + 
> +  const TargetRegisterInfo *TRI = getTargetRegisterInfo();
> +  // TODO: This could be more efficient by bulk changing the operands.
> +  for (reg_iterator I = reg_begin(FromReg), E = reg_end(); I != E; ) {
> +    MachineOperand &O = *I;
> +    ++I;
> +    if (O.getSubReg()) {
> +      unsigned SubReg = TRI->getSubReg(ToReg, O.getSubReg());
> +      assert(SubReg);
> +      setPhysRegUsed(SubReg);
> +      O.setReg(SubReg);
> +      O.setSubReg(0);
> +    } else {
> +      setPhysRegUsed(ToReg);
> +      O.setReg(ToReg);
> +    }
> +  }
> +}

As noted above, this is duplicating MachineOperand::substPhysReg().

Also, I don’t think this is the right place to call setPhysRegUsed() since other MRI functions don’t do that either. Just call it from the client.

> @@ -888,7 +889,7 @@
>           // Replace this reference to the virtual register with the
>           // scratch register.
>           assert (ScratchReg && "Missing scratch register!");
> -          Fn.getRegInfo().replaceRegWith(Reg, ScratchReg);
> +          Fn.getRegInfo().replaceVRegWithPhysReg(Reg, ScratchReg);

As noted above, this change shouldn’t be necessary.

> +void RegScavenger::addRegUnits(BitVector &BV, BitVector &RBV) {
> +  for (int Reg = RBV.find_first(); Reg != -1; Reg = RBV.find_next(Reg))
> +    if (Reg)
> +      addRegUnits(BV, Reg);
> +}
> +
> +void RegScavenger::subRegUnits(BitVector &BV, BitVector &RBV) {
> +  for (int Reg = RBV.find_first(); Reg != -1; Reg = RBV.find_next(Reg))
> +    if (Reg)
> +      subRegUnits(BV, Reg);
> +}

These functions look potentially expensive, and the if(Reg) check doesn’t seem right. Why would bit 0 be set? More below.

> @@ -122,12 +139,42 @@
>   // predicated, conservatively assume "kill" markers do not actually kill the
>   // register. Similarly ignores "dead" markers.
>   bool isPred = TII->isPredicated(MI);
> -  KillRegs.reset();
> -  DefRegs.reset();
> +  KillRegUnits.reset();
> +  DefRegUnits.reset();
>   for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
>     const MachineOperand &MO = MI->getOperand(i);
> -    if (MO.isRegMask())
> -      (isPred ? DefRegs : KillRegs).setBitsNotInMask(MO.getRegMask());
> +    if (MO.isRegMask()) {
> +      
> +      // The code below is needed to obtain a proper set of reg units that are
> +      // trully clobbered. As an example consider ARM64 registers X18, X19 and
> +      // the "register" W18_W19. The X18 is not preserved while X19 is;
> +      // therefore W18_W19 is considered clobbered but it has inside it a reg
> +      // unit that is trully preserved (the reg unit for X19). Hence the logic:
> +      //
> +      // ClobberedRegisterUnits - TrullyPreservedRegisterUnits =
> +      // TrullyClobberedRegisterUnits
> +      //
> +      // Is needed in order to obtain a conservative approximation of the
> +      // set that is trully clobbered by the instruction.
> +      
> +      // Get the clobbered registers.
> +      TmpRegs.setBitsInMask(MO.getRegMask());
> +      TmpRegs.flip();
> +      
> +      TmpRegUnits.reset();
> +      
> +      // Add all the clobbered registers as reg units.
> +      addRegUnits(TmpRegUnits, TmpRegs);
> +      
> +      // Get the preserved registers.
> +      TmpRegs.setBitsInMask(MO.getRegMask());
> +      
> +      // Remove all the preserved reg units.
> +      subRegUnits(TmpRegUnits, TmpRegs);
> +      
> +      // Apply the mask.
> +      (isPred ? DefRegUnits : KillRegUnits) |= TmpRegUnits;
> +    }

This looks quite expensive, the addRegUnits / subRegUnits are both O(NumRegUnits * NumRegs).

It should be a lot faster to check if the root registers are preserved for each regunit.


> void RegScavenger::getRegsUsed(BitVector &used, bool includeReserved) {
> -  used = RegsAvailable;
> -  used.flip();
> +  TmpRegUnits = RegUnitsAvailable;
> +  TmpRegUnits.flip();

I don’t think any callers actually need this whole vector, and it is quite expensive to compute. You should remove this function and change the callers to use isRegUsed() instead.

Thanks,
/jakob