[PEI] Remove required properties and use 'if' instead of std::function

[rnk]

Summary

After r303360, we initialize UsesCalleeSaves in runOnMachineFunction,
which runs after getRequiredProperties. UsesCalleeSaves was initialized
to 'false', so getRequiredProperties would always return an empty set.
We don't have a TargetMachine available early anymore after r303360.
Just removing the requirement of NoVRegs seems to make things work, so
let's do that.

[dschuff]

Removing the requirement works fine now because the passes are set up correctly. The original intention behind getRequiredProperties in this case was that if the pass configuration were to be changed and PEI were to be called before regalloc on a target which wasn't prepared for that, there would be an easy assertion to say what went wrong.

We could just add a conditional assertion in runOnMachineFunction to ensure things are correct; in fact my original patch to PEI for WebAssembly more or less did that. But my reviewer (@qcolombet ) really wanted a stronger mechanism to enforce that kind of invariant and IIRC that's why getRequiredProperties was born.

I guess if we don't have the target anymore at the right time, we can't really properly set getRequiredProperties. I'd be fine with going back to a simple assertion here, but I guess that risks re-opening the debate :)

llvm/lib/CodeGen/PrologEpilogInserter.cpp

82 ↗

(On Diff #117874)

regardless of what we decide about getRequiredProperties we should do the rest of this change (i.e. this and below). There was some discussion about it after it originally landed and it was my intention to do more or less exactly this part of your patch, and I didn't get around to it :(

[MatzeB]

+1. You could still check the condition in an assert() at the right place though.

[rnk]

• Try to add the right assert

[MatzeB]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

155 ↗

(On Diff #117874)

I'd name the variable UsesPhysRegsForPEI.

174 ↗

(On Diff #117874)

With the std::function gone we can rename doSpillCalleeSavedRegs back to spillCalleeSavedRegs

201–205 ↗

(On Diff #117874)

This feels wrong, we should only clearVirtRegs() when we actually scavenged them. Checking the code it seems that the clearing is already done in scavengeFrameVirtualRegs() nowadays so we can remove it here. Maybe we should also skip the whole if (UsesCalleeSaves) then too as it seems target can already control this bit via TRI::requiresRegisterScavenging`.

[dschuff]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

201–205 ↗

(On Diff #117874)

+1 in general. I'm ambivalent on removing if (UsesCalleeSaves) altogether. It's true that no target I can imagine would have UsesCalleeSaves be false and requiresRegisterScavenging be true, but having it explicit makes it a bit more obvious. I'd be OK either way I think.

492 ↗

(On Diff #117880)

I think this is the right assertion, but there should be a comment here about why we are doing it manually instead of using getRequiredProperties, since that would be the expected thing.

[rnk]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

201–205 ↗

(On Diff #117874)

I agree. Most targets don't need register scavenging, and wasm (the only target that uses vregs past this point) definitely doesn't need it either.

[rnk]

• more simplifications

[MatzeB]

LGTM; maybe wait a bit for @qcolombet to comment.

[rnk]

• comment

[rnk]

I know I've only waited a day for @qcolombet and that's not much time, but I'm going to land this because the requirements checking change actually doesn't change our current functionality. Before this change, UsesCalleeSaves is initialized to false, so PEI doesn't require NoVRegs. This will actually give us better runtime checking.

[qcolombet]

Sorry Reid @rnk, I didn't know you were waiting for me. I saw already two people were saying it's good and I thought we didn't need a third one :).

It looks good to me too!

llvm/trunk/lib/CodeGen/PrologEpilogInserter.cpp

 #include "llvm/Target/TargetSubtargetInfo.h"
 #include <climits>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "prologepilog"
 
 typedef SmallVector<MachineBasicBlock *, 4> MBBVector;
-static void doSpillCalleeSavedRegs(MachineFunction &MF, RegScavenger *RS,
-                                   unsigned &MinCSFrameIndex,
-                                   unsigned &MaxCXFrameIndex,
-                                   const MBBVector &SaveBlocks,
-                                   const MBBVector &RestoreBlocks);
+static void spillCalleeSavedRegs(MachineFunction &MF, RegScavenger *RS,
 
 namespace {
 class PEI : public MachineFunctionPass {
 public:
   static char ID;
   PEI() : MachineFunctionPass(ID) {
     initializePEIPass(*PassRegistry::getPassRegistry());
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
-  MachineFunctionProperties getRequiredProperties() const override {
-    MachineFunctionProperties MFP;
-    if (UsesCalleeSaves)
-      MFP.set(MachineFunctionProperties::Property::NoVRegs);
-    return MFP;
-  }
-
   /// runOnMachineFunction - Insert prolog/epilog code and replace abstract
   /// frame indexes with appropriate references.
   ///
   bool runOnMachineFunction(MachineFunction &Fn) override;
 
 private:
-  std::function<void(MachineFunction &MF, RegScavenger *RS,
-                     unsigned &MinCSFrameIndex, unsigned &MaxCSFrameIndex,
-                     const MBBVector &SaveBlocks,
-                     const MBBVector &RestoreBlocks)>
-      SpillCalleeSavedRegisters;
-  std::function<void(MachineFunction &MF, RegScavenger &RS)>
-      ScavengeFrameVirtualRegs;
-
-  bool UsesCalleeSaves = false;
-
   RegScavenger *RS;
 
   // MinCSFrameIndex, MaxCSFrameIndex - Keeps the range of callee saved
   // stack frame indexes.
   unsigned MinCSFrameIndex = std::numeric_limits<unsigned>::max();
   unsigned MaxCSFrameIndex = 0;
 
   // Save and Restore blocks of the current function. Typically there is a

 
 /// StackObjSet - A set of stack object indexes
 typedef SmallSetVector<int, 8> StackObjSet;
 
 /// runOnMachineFunction - Insert prolog/epilog code and replace abstract
 /// frame indexes with appropriate references.
 ///
 bool PEI::runOnMachineFunction(MachineFunction &Fn) {
-  if (!SpillCalleeSavedRegisters) {
-    const TargetMachine &TM = Fn.getTarget();
-    if (!TM.usesPhysRegsForPEI()) {
-      SpillCalleeSavedRegisters = [](MachineFunction &, RegScavenger *,
-                                     unsigned &, unsigned &, const MBBVector &,
-                                     const MBBVector &) {};
-      ScavengeFrameVirtualRegs = [](MachineFunction &, RegScavenger &) {};
-    } else {
-      SpillCalleeSavedRegisters = doSpillCalleeSavedRegs;
-      ScavengeFrameVirtualRegs = scavengeFrameVirtualRegs;
-      UsesCalleeSaves = true;
-    }
-  }
-
   const Function* F = Fn.getFunction();
   const TargetRegisterInfo *TRI = Fn.getSubtarget().getRegisterInfo();
   const TargetFrameLowering *TFI = Fn.getSubtarget().getFrameLowering();
 
   RS = TRI->requiresRegisterScavenging(Fn) ? new RegScavenger() : nullptr;
   FrameIndexVirtualScavenging = TRI->requiresFrameIndexScavenging(Fn);
   FrameIndexEliminationScavenging = (RS && !FrameIndexVirtualScavenging) ||
     TRI->requiresFrameIndexReplacementScavenging(Fn);
   ORE = &getAnalysis<MachineOptimizationRemarkEmitterPass>().getORE();
 
   // Calculate the MaxCallFrameSize and AdjustsStack variables for the
   // function's frame information. Also eliminates call frame pseudo
   // instructions.
   calculateCallFrameInfo(Fn);
 
   // Determine placement of CSR spill/restore code and prolog/epilog code:
   // place all spills in the entry block, all restores in return blocks.
   calculateSaveRestoreBlocks(Fn);
 
   // Handle CSR spilling and restoring, for targets that need it.
-  SpillCalleeSavedRegisters(Fn, RS, MinCSFrameIndex, MaxCSFrameIndex,
-                            SaveBlocks, RestoreBlocks);
+  if (Fn.getTarget().usesPhysRegsForPEI())
+    spillCalleeSavedRegs(Fn, RS, MinCSFrameIndex, MaxCSFrameIndex, SaveBlocks,
+                         RestoreBlocks);
 
   // Allow the target machine to make final modifications to the function
   // before the frame layout is finalized.
   TFI->processFunctionBeforeFrameFinalized(Fn, RS);
 
   // Calculate actual frame offsets for all abstract stack objects...
   calculateFrameObjectOffsets(Fn);
 
   // Add prolog and epilog code to the function.  This function is required
   // to align the stack frame as necessary for any stack variables or
   // called functions.  Because of this, calculateCalleeSavedRegisters()
   // must be called before this function in order to set the AdjustsStack
   // and MaxCallFrameSize variables.
   if (!F->hasFnAttribute(Attribute::Naked))
     insertPrologEpilogCode(Fn);
 
   // Replace all MO_FrameIndex operands with physical register references
   // and actual offsets.
   //
   replaceFrameIndices(Fn);
 
   // If register scavenging is needed, as we've enabled doing it as a
   // post-pass, scavenge the virtual registers that frame index elimination
   // inserted.
-  if (TRI->requiresRegisterScavenging(Fn) && FrameIndexVirtualScavenging) {
-      ScavengeFrameVirtualRegs(Fn, *RS);
-
-      // Clear any vregs created by virtual scavenging.
-      Fn.getRegInfo().clearVirtRegs();
-  }
+  if (TRI->requiresRegisterScavenging(Fn) && FrameIndexVirtualScavenging)
+    scavengeFrameVirtualRegs(Fn, *RS);
 
   // Warn on stack size when we exceeds the given limit.
   MachineFrameInfo &MFI = Fn.getFrameInfo();
   uint64_t StackSize = MFI.getStackSize();
   if (WarnStackSize.getNumOccurrences() > 0 && WarnStackSize < StackSize) {
     DiagnosticInfoStackSize DiagStackSize(*F, StackSize);
     F->getContext().diagnose(DiagStackSize);
   }
for (const CalleeSavedInfo &CI : reverse(CSI)) {
       assert(I != RestoreBlock.begin() &&
              "loadRegFromStackSlot didn't insert any code!");
       // Insert in reverse order.  loadRegFromStackSlot can insert
       // multiple instructions.
     }
   }
 }
 
-static void doSpillCalleeSavedRegs(MachineFunction &Fn, RegScavenger *RS,
-                                   unsigned &MinCSFrameIndex,
-                                   unsigned &MaxCSFrameIndex,
-                                   const MBBVector &SaveBlocks,
-                                   const MBBVector &RestoreBlocks) {
+static void spillCalleeSavedRegs(MachineFunction &Fn, RegScavenger *RS,
+  // We can't list this requirement in getRequiredProperties because some
+  // targets (WebAssembly) use virtual registers past this point, and the pass
+  // pipeline is set up without giving the passes a chance to look at the
+  // TargetMachine.
+  // FIXME: Find a way to express this in getRequiredProperties.
+  assert(Fn.getProperties().hasProperty(
+      MachineFunctionProperties::Property::NoVRegs));
+
   const Function *F = Fn.getFunction();
   const TargetFrameLowering *TFI = Fn.getSubtarget().getFrameLowering();
   MachineFrameInfo &MFI = Fn.getFrameInfo();
   MinCSFrameIndex = std::numeric_limits<unsigned>::max();
   MaxCSFrameIndex = 0;
 
   // Determine which of the registers in the callee save list should be saved.
   BitVector SavedRegs;