[RegAlloc] Don’t freeze reserved registers again if it is not necessary

[volkan]

Summary

SelectionDAGISel calls MachineRegisterInfo::freezeReservedRegs
right after ISel and MachineRegisterInfo caches the reserved registers
vector. However, RegAlloc calls the same function again before the
allocation process. This might be expensive if the target has a large
number of registers.

The second call was removed in r168630 in 2012, but it caused a few
failures and got reverted. They don’t fail right now, but there are 4
PowerPC tests failing. So, it seems some targets still need to update
the reserved registers before RegAlloc.
Failing Tests (4):

LLVM :: CodeGen/PowerPC/ppc64-anyregcc-crash.ll
LLVM :: CodeGen/PowerPC/ppc64-anyregcc.ll
LLVM :: CodeGen/PowerPC/ppc64-patchpoint.ll
LLVM :: CodeGen/PowerPC/ppc64-stackmap.ll

[nemanjai]

I think that [some] target maintainers should be on the list of reviewers if you hope to get this patch reviewed/committed. You can get a list of target maintainers from the CODE_OWNERS.TXT at the top level llvm source directory.

include/llvm/Target/TargetRegisterInfo.h

822

I think a comment with an example of why this may be needed by a target is in order here. For example, PPC requires it because it has a different set of reserved registers if a function ends up having a TargetOpcode::PATCHPOINT node in it.
Perhaps @hfinkel has a different suggestion here.

825

I am also not convinced that the default here should be to return false. The fact that PowerPC is the only target that has failures in the unit tests without the second call to MachineRegisterInfo::freezeReservedRegs could certainly be because PowerPC is the only target that needs it. But it could just as well be the case that it's just the only target that actually has a unit test that is sensitive to this.
It seems to me that the better approach would be to conservatively have both calls by default. Then allow targets that want to save on compile time and don't need both calls, to override this and not get both calls.

[qcolombet]

include/llvm/Target/TargetRegisterInfo.h

822

My understanding is that this kind of things should be settled at ISel time. I.e., I would expect only one call to the freeze function per function. If we need more, I'd say our representation is broken.

I'll dig a little more to make sure we are not missing anything (in particular I'll have a look at r168630), but ultimately, I would suggest that all targets work with just one call to this.

[MatzeB]

• Calling a freeze function and later changing the set of reserved registers intuitively seems wrong to me. I wonder whether this isn't more accidental than actually planned. The name freeze indicates to me that the information will not change anymore; What if early passes make decisions based on an incomplete reserved set, can we get into trouble? In any case I'd really like to understand why targets rely on changing the reserved register set later.
• While I am in favor to push things to a single call to freezeReservedRegs() I have no strong opinion on when this call happens; Do early passes actually need a notion of reserved registers?
• I'm not a fan of adding more and more target callbacks. We should first try hard to understand what is going on here and try to come to a solution where all targets behave the same without the need for a callback. Even if we figure out some targets need special handling we could employ existing mechanisms like having the target insert a pass at the apropriate part of the pipeline to re-freeze the registers.

[nemanjai]

include/llvm/Target/TargetRegisterInfo.h

822

The relevant comment from the PPC back end at the location where the missing reserved register is reconciled is in PPCISelLowering.cpp and reads:

// Call lowering should have added an r2 operand to indicate a dependence
// on the TOC base pointer value. It can't however, because there is no
// way to mark the dependence as implicit there, and so the stackmap code
// will confuse it with a regular operand. Instead, add the dependence
// here.

My initial comment was that we shouldn't have a target query for this but should just resolve whatever is preventing PPC from doing the right thing at ISEL time. But that comment seemed made me change my mind.
If we can get that fixed in the PPC back end, I'm in favour of that solution over a target query.

[qcolombet]

What happen if we just freeze in the regalloc code?
Basically, are there any other pass that needs to know about that?

include/llvm/Target/TargetRegisterInfo.h

822

Do you know what is missing to make that possible?

[arsenm]

In D29448#672445, @qcolombet wrote:

What happen if we just freeze in the regalloc code?
Basically, are there any other pass that needs to know about that?

This would be greatly preferable. I ran into this problem before. I think it's important to freeze this in regalloc because currently you can't make decisions in ExpandISelPseudos that change the set of reserved registers. Where it's called the first time is too early

[volkan]

In D29448#672445, @qcolombet wrote:

What happen if we just freeze in the regalloc code?
Basically, are there any other pass that needs to know about that?

Yes, this cause a lot of failures. DeadMachineInstructionElim, LiveVariables and MachineVerifier use reserved register.

[qcolombet]

Hi Volkan,

I think LiveVariables is not used anymore and the MachineVerifier could certainly be adapted.

Could you check how DeadMachineInstructionElim uses that information and how we can live without the reserved set to be frozen?

Cheers,
-Quentin

[MatzeB]

In D29448#679472, @qcolombet wrote:

Hi Volkan,

I think LiveVariables is not used anymore and the MachineVerifier could certainly be adapted.

I wish that were true :)

However it is used pretty early in the compilation process (for PHIElimination and TwoAddressInstructions) we may or may not not need to freeze registers that early...

include/llvm/Target/TargetRegisterInfo.h

public:
   }
 
   /// Returns true if the target wants the LocalStackAllocation pass to be run
   /// and virtual base registers used for more efficient stack access.
   virtual bool requiresVirtualBaseRegisters(const MachineFunction &MF) const {
     return false;
   }
 
+  /// Returns true if the target wants to update the reserved register
+  /// before the register allocation process.
+  virtual bool
+  requiresFreezingReservedRegsBeforeRegAlloc(const MachineFunction &MF) const {
+    return false;
+  }
+
   /// Return true if target has reserved a spill slot in the stack frame of
   /// the given function for the specified register. e.g. On x86, if the frame
   /// register is required, the first fixed stack object is reserved as its
   /// spill slot. This tells PEI not to create a new stack frame
   /// object for the given register. It should be called only after
   /// determineCalleeSaves().
   virtual bool hasReservedSpillSlot(const MachineFunction &MF, unsigned Reg,
                                     int &FrameIdx) const {

lib/CodeGen/RegAllocBase.cpp

 void RegAllocBase::init(VirtRegMap &vrm,
                         LiveIntervals &lis,
                         LiveRegMatrix &mat) {
   TRI = &vrm.getTargetRegInfo();
   MRI = &vrm.getRegInfo();
   VRM = &vrm;
   LIS = &lis;
   Matrix = &mat;
-  MRI->freezeReservedRegs(vrm.getMachineFunction());
-  RegClassInfo.runOnMachineFunction(vrm.getMachineFunction());
+  MachineFunction &MF = VRM->getMachineFunction();
+  if (TRI->requiresFreezingReservedRegsBeforeRegAlloc(MF))
+    MRI->freezeReservedRegs(MF);
+  RegClassInfo.runOnMachineFunction(MF);
 }
 
 // Visit all the live registers. If they are already assigned to a physical
 // register, unify them with the corresponding LiveIntervalUnion, otherwise push
 // them on the priority queue for later assignment.
 void RegAllocBase::seedLiveRegs() {
   NamedRegionTimer T("seed", "Seed Live Regs", TimerGroupName,
                      TimerGroupDescription, TimePassesIsEnabled);

lib/CodeGen/RegAllocFast.cpp

 ///
 bool RAFast::runOnMachineFunction(MachineFunction &Fn) {
   DEBUG(dbgs() << "********** FAST REGISTER ALLOCATION **********\n"
                << "********** Function: " << Fn.getName() << '\n');
   MF = &Fn;
   MRI = &MF->getRegInfo();
   TRI = MF->getSubtarget().getRegisterInfo();
   TII = MF->getSubtarget().getInstrInfo();
+  if (TRI->requiresFreezingReservedRegsBeforeRegAlloc(Fn))
     MRI->freezeReservedRegs(Fn);
   RegClassInfo.runOnMachineFunction(Fn);
   UsedInInstr.clear();
   UsedInInstr.setUniverse(TRI->getNumRegUnits());
 
   // initialize the virtual->physical register map to have a 'null'
   // mapping for all virtual registers
   StackSlotForVirtReg.resize(MRI->getNumVirtRegs());
   LiveVirtRegs.setUniverse(MRI->getNumVirtRegs());

lib/CodeGen/RegAllocPBQP.cpp

bool RegAllocPBQP::runOnMachineFunction(MachineFunction &MF) {
 
   VirtRegMap &VRM = getAnalysis<VirtRegMap>();
 
   calculateSpillWeightsAndHints(LIS, MF, &VRM, getAnalysis<MachineLoopInfo>(),
                                 MBFI, normalizePBQPSpillWeight);
 
   std::unique_ptr<Spiller> VRegSpiller(createInlineSpiller(*this, MF, VRM));
 
+  const TargetRegisterInfo *TRI = MF.getSubtarget().getRegisterInfo();
+  if (TRI->requiresFreezingReservedRegsBeforeRegAlloc(MF))
     MF.getRegInfo().freezeReservedRegs(MF);
 
   DEBUG(dbgs() << "PBQP Register Allocating for " << MF.getName() << "\n");
 
   // Allocator main loop:
   //
   // * Map current regalloc problem to a PBQP problem
   // * Solve the PBQP problem
   // * Map the solution back to a register allocation

lib/Target/PowerPC/PPCRegisterInfo.h

public:
   bool trackLivenessAfterRegAlloc(const MachineFunction &MF) const override {
     return true;
   }
 
   bool requiresVirtualBaseRegisters(const MachineFunction &MF) const override {
     return true;
   }
 
+  bool requiresFreezingReservedRegsBeforeRegAlloc(
+      const MachineFunction &MF) const override {
+    return true;
+  }
+
   void lowerDynamicAlloc(MachineBasicBlock::iterator II) const;
   void lowerDynamicAreaOffset(MachineBasicBlock::iterator II) const;
   void lowerCRSpilling(MachineBasicBlock::iterator II,
                        unsigned FrameIndex) const;
   void lowerCRRestore(MachineBasicBlock::iterator II,
                       unsigned FrameIndex) const;
   void lowerCRBitSpilling(MachineBasicBlock::iterator II,
                           unsigned FrameIndex) const;