AArch64: Schedule DeadRegisterDefinitionsPass before regalloc.

[MatzeB]

Summary

Doing this before register allocation reduces register pressure as we do
not even have to allocate a register for those dead definitions.

(Requires D26106 to avoid some accidental regressions).

I am running tests on the effects now and add a comment here.

[MatzeB]

Turns out you also need D26221 to avoid some regressions with this.

[gberry]

This mostly looks good to me, though I have a few questions below.

lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp

92 ↗

(On Diff #76287)

Perhaps you could add an assert here checking this?

96 ↗

(On Diff #76287)

Is this use_nodbg_empty check necessary? Can we end up with virtual registers marked dead with dbg uses?

148 ↗

(On Diff #76287)

Perhaps you should commit this bug fix (local Changed shadowing the member) separately?

[MatzeB]

lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp

92 ↗

(On Diff #76287)

I'd rather not, as it is technically legal to do so. This is merely an explanation why I did not add code to handle the physreg case; and I'd rather not add that code as it would be unused and hence untested today.

96 ↗

(On Diff #76287)

This is not about debug values (that function ignores any debug uses), but a way to detect dead definitions that do not have the Dead flag set.

I encountered several instances of dead defs without a dead flag set. I am not entirely sure whether the missing Dead flags are a bug or whether they need not be exact pre-regalloc. In any case you can see that DeadMachineInstructionElim does the same check to determine whether a def is dead.

148 ↗

(On Diff #76287)

ok.

[MatzeB]

ping

[qcolombet]

Hi Matthias,

LGTM.
One comment, could you add a test case where this save spill?
I guess it requires a bit of work given we need to expose case with spill, but if you can come up with something reasonably small, go for it!
Otherwise, I guess it is already well covered by the exiting tests.

Cheers,
-Quentin

[MatzeB]

In D26111#596632, @qcolombet wrote:

Hi Matthias,

LGTM.
One comment, could you add a test case where this save spill?

sure.

llvm/trunk/lib/Target/AArch64/AArch64DeadRegisterDefinitionsPass.cpp

 //===----------------------------------------------------------------------===//
 
 #include "AArch64.h"
 #include "AArch64RegisterInfo.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstr.h"
+#include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
+#include "llvm/Target/TargetInstrInfo.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "aarch64-dead-defs"
 
 STATISTIC(NumDeadDefsReplaced, "Number of dead definitions replaced");
 
 #define AARCH64_DEAD_REG_DEF_NAME "AArch64 Dead register definitions"
 
 namespace {
 class AArch64DeadRegisterDefinitions : public MachineFunctionPass {
 private:
   const TargetRegisterInfo *TRI;
+  const MachineRegisterInfo *MRI;
+  const TargetInstrInfo *TII;
   bool Changed;
-  bool implicitlyDefinesOverlappingReg(unsigned Reg, const MachineInstr &MI);
   void processMachineBasicBlock(MachineBasicBlock &MBB);
 public:
   static char ID; // Pass identification, replacement for typeid.
   AArch64DeadRegisterDefinitions() : MachineFunctionPass(ID) {
     initializeAArch64DeadRegisterDefinitionsPass(
         *PassRegistry::getPassRegistry());
   }
 
   bool runOnMachineFunction(MachineFunction &F) override;
 
-  MachineFunctionProperties getRequiredProperties() const override {
-    return MachineFunctionProperties().set(
-        MachineFunctionProperties::Property::NoVRegs);
-  }
-
   StringRef getPassName() const override { return AARCH64_DEAD_REG_DEF_NAME; }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     MachineFunctionPass::getAnalysisUsage(AU);
   }
 };
 char AArch64DeadRegisterDefinitions::ID = 0;
 } // end anonymous namespace
 
 INITIALIZE_PASS(AArch64DeadRegisterDefinitions, "aarch64-dead-defs",
                 AARCH64_DEAD_REG_DEF_NAME, false, false)
 
-bool AArch64DeadRegisterDefinitions::implicitlyDefinesOverlappingReg(
-    unsigned Reg, const MachineInstr &MI) {
-  for (const MachineOperand &MO : MI.implicit_operands())
-    if (MO.isReg() && MO.isDef())
-      if (TRI->regsOverlap(Reg, MO.getReg()))
-        return true;
-  return false;
-}
-
 static bool usesFrameIndex(const MachineInstr &MI) {
   for (const MachineOperand &MO : MI.uses())
     if (MO.isFI())
       return true;
   return false;
 }
 
 void AArch64DeadRegisterDefinitions::processMachineBasicBlock(
     MachineBasicBlock &MBB) {
+  const MachineFunction &MF = *MBB.getParent();
   for (MachineInstr &MI : MBB) {
     if (usesFrameIndex(MI)) {
       // We need to skip this instruction because while it appears to have a
       // dead def it uses a frame index which might expand into a multi
       // instruction sequence during EPI.
       DEBUG(dbgs() << "    Ignoring, operand is frame index\n");
       continue;
     }
     if (MI.definesRegister(AArch64::XZR) || MI.definesRegister(AArch64::WZR)) {
       // It is not allowed to write to the same register (not even the zero
       // register) twice in a single instruction.
       DEBUG(dbgs() << "    Ignoring, XZR or WZR already used by the instruction\n");
       continue;
     }
     const MCInstrDesc &Desc = MI.getDesc();
     for (int I = 0, E = Desc.getNumDefs(); I != E; ++I) {
       MachineOperand &MO = MI.getOperand(I);
-      if (!MO.isReg() || !MO.isDead() || !MO.isDef())
+      if (!MO.isReg() || !MO.isDef())
+        continue;
+      // We should not have any relevant physreg defs that are replacable by
+      // zero before register allocation. So we just check for dead vreg defs.
+      unsigned Reg = MO.getReg();
+      if (!TargetRegisterInfo::isVirtualRegister(Reg) ||
+          (!MO.isDead() && !MRI->use_nodbg_empty(Reg)))
         continue;
       assert(!MO.isImplicit() && "Unexpected implicit def!");
       DEBUG(dbgs() << "  Dead def operand #" << I << " in:\n    ";
             MI.print(dbgs()));
       // Be careful not to change the register if it's a tied operand.
       if (MI.isRegTiedToUseOperand(I)) {
         DEBUG(dbgs() << "    Ignoring, def is tied operand.\n");
         continue;
       }
-      // Don't change the register if there's an implicit def of a subreg or
-      // superreg.
-      if (implicitlyDefinesOverlappingReg(MO.getReg(), MI)) {
-        DEBUG(dbgs() << "    Ignoring, implicitly defines overlap reg.\n");
-        continue;
-      }
-      // Make sure the instruction take a register class that contains
-      // the zero register and replace it if so.
+      const TargetRegisterClass *RC = TII->getRegClass(Desc, I, TRI, MF);
       unsigned NewReg;
-      switch (Desc.OpInfo[I].RegClass) {
-      default:
+      if (RC == nullptr) {
         DEBUG(dbgs() << "    Ignoring, register is not a GPR.\n");
         continue;
-      case AArch64::GPR32RegClassID:
+      } else if (RC->contains(AArch64::WZR))
         NewReg = AArch64::WZR;
-        break;
-      case AArch64::GPR64RegClassID:
+      else if (RC->contains(AArch64::XZR))
         NewReg = AArch64::XZR;
-        break;
+      else {
+        DEBUG(dbgs() << "    Ignoring, register is not a GPR.\n");
+        continue;
       }
       DEBUG(dbgs() << "    Replacing with zero register. New:\n      ");
       MO.setReg(NewReg);
+      MO.setIsDead();
       DEBUG(MI.print(dbgs()));
       ++NumDeadDefsReplaced;
       Changed = true;
       // Only replace one dead register, see check for zero register above.
       break;
     }
   }
 }
 
 // Scan the function for instructions that have a dead definition of a
 // register. Replace that register with the zero register when possible.
 bool AArch64DeadRegisterDefinitions::runOnMachineFunction(MachineFunction &MF) {
   if (skipFunction(*MF.getFunction()))
     return false;
 
   TRI = MF.getSubtarget().getRegisterInfo();
+  TII = MF.getSubtarget().getInstrInfo();
+  MRI = &MF.getRegInfo();
   DEBUG(dbgs() << "***** AArch64DeadRegisterDefinitions *****\n");
   Changed = false;
   for (auto &MBB : MF)
     processMachineBasicBlock(MBB);
   return Changed;
 }
 
 FunctionPass *llvm::createAArch64DeadRegisterDefinitions() {
   return new AArch64DeadRegisterDefinitions();
 }

llvm/trunk/lib/Target/AArch64/AArch64TargetMachine.cpp

if (EnableEarlyIfConversion)
     addPass(&EarlyIfConverterID);
   if (EnableStPairSuppress)
     addPass(createAArch64StorePairSuppressPass());
   addPass(createAArch64VectorByElementOptPass());
   return true;
 }
 
 void AArch64PassConfig::addPreRegAlloc() {
+  // Change dead register definitions to refer to the zero register.
+  if (TM->getOptLevel() != CodeGenOpt::None && EnableDeadRegisterElimination)
+    addPass(createAArch64DeadRegisterDefinitions());
+
   // Use AdvSIMD scalar instructions whenever profitable.
   if (TM->getOptLevel() != CodeGenOpt::None && EnableAdvSIMDScalar) {
     addPass(createAArch64AdvSIMDScalar());
     // The AdvSIMD pass may produce copies that can be rewritten to
     // be register coaleascer friendly.
     addPass(&PeepholeOptimizerID);
   }
 }
 
 void AArch64PassConfig::addPostRegAlloc() {
   // Remove redundant copy instructions.
   if (TM->getOptLevel() != CodeGenOpt::None && EnableRedundantCopyElimination)
     addPass(createAArch64RedundantCopyEliminationPass());
 
-  // Change dead register definitions to refer to the zero register.
-  if (TM->getOptLevel() != CodeGenOpt::None && EnableDeadRegisterElimination)
-    addPass(createAArch64DeadRegisterDefinitions());
   if (TM->getOptLevel() != CodeGenOpt::None && usingDefaultRegAlloc())
     // Improve performance for some FP/SIMD code for A57.
     addPass(createAArch64A57FPLoadBalancing());
 }
 
 void AArch64PassConfig::addPreSched2() {
   // Expand some pseudo instructions to allow proper scheduling.
   addPass(createAArch64ExpandPseudoPass());

llvm/trunk/test/CodeGen/AArch64/arm64-zeroreg.ll

+; RUN: llc -o - %s | FileCheck %s
+target triple = "aarch64--"
+
+declare void @begin()
+declare void @end()
+
+; Test that we use the zero register before regalloc and do not unnecessarily
+; clobber a register with the SUBS (cmp) instruction.
+; CHECK-LABEL: func:
+define void @func(i64* %addr) {
+  ; We should not see any spills or reloads between begin and end
+  ; CHECK: bl begin
+  ; CHECK-NOT: str{{.*}}sp
+  ; CHECK-NOT: Folded Spill
+  ; CHECK-NOT: ldr{{.*}}sp
+  ; CHECK-NOT: Folded Reload
+  call void @begin()
+  %v0 = load volatile i64, i64* %addr  
+  %v1 = load volatile i64, i64* %addr  
+  %v2 = load volatile i64, i64* %addr  
+  %v3 = load volatile i64, i64* %addr  
+  %v4 = load volatile i64, i64* %addr  
+  %v5 = load volatile i64, i64* %addr  
+  %v6 = load volatile i64, i64* %addr  
+  %v7 = load volatile i64, i64* %addr  
+  %v8 = load volatile i64, i64* %addr  
+  %v9 = load volatile i64, i64* %addr  
+  %v10 = load volatile i64, i64* %addr  
+  %v11 = load volatile i64, i64* %addr  
+  %v12 = load volatile i64, i64* %addr  
+  %v13 = load volatile i64, i64* %addr  
+  %v14 = load volatile i64, i64* %addr  
+  %v15 = load volatile i64, i64* %addr  
+  %v16 = load volatile i64, i64* %addr  
+  %v17 = load volatile i64, i64* %addr  
+  %v18 = load volatile i64, i64* %addr  
+  %v19 = load volatile i64, i64* %addr  
+  %v20 = load volatile i64, i64* %addr
+  %v21 = load volatile i64, i64* %addr
+  %v22 = load volatile i64, i64* %addr
+  %v23 = load volatile i64, i64* %addr
+  %v24 = load volatile i64, i64* %addr
+  %v25 = load volatile i64, i64* %addr
+  %v26 = load volatile i64, i64* %addr
+  %v27 = load volatile i64, i64* %addr
+  %v28 = load volatile i64, i64* %addr
+  %v29 = load volatile i64, i64* %addr
+
+  %c = icmp eq i64 %v0, %v1
+  br i1 %c, label %if.then, label %if.end
+
+if.then:
+  store volatile i64 %v2, i64* %addr
+  br label %if.end
+
+if.end:
+  store volatile i64 %v0, i64* %addr
+  store volatile i64 %v1, i64* %addr
+  store volatile i64 %v2, i64* %addr
+  store volatile i64 %v3, i64* %addr
+  store volatile i64 %v4, i64* %addr
+  store volatile i64 %v5, i64* %addr
+  store volatile i64 %v6, i64* %addr
+  store volatile i64 %v7, i64* %addr
+  store volatile i64 %v8, i64* %addr
+  store volatile i64 %v9, i64* %addr
+  store volatile i64 %v10, i64* %addr
+  store volatile i64 %v11, i64* %addr
+  store volatile i64 %v12, i64* %addr
+  store volatile i64 %v13, i64* %addr
+  store volatile i64 %v14, i64* %addr
+  store volatile i64 %v15, i64* %addr
+  store volatile i64 %v16, i64* %addr
+  store volatile i64 %v17, i64* %addr
+  store volatile i64 %v18, i64* %addr
+  store volatile i64 %v19, i64* %addr
+  store volatile i64 %v20, i64* %addr
+  store volatile i64 %v21, i64* %addr
+  store volatile i64 %v22, i64* %addr
+  store volatile i64 %v23, i64* %addr
+  store volatile i64 %v24, i64* %addr
+  store volatile i64 %v25, i64* %addr
+  store volatile i64 %v26, i64* %addr
+  store volatile i64 %v27, i64* %addr
+  store volatile i64 %v28, i64* %addr
+  store volatile i64 %v29, i64* %addr
+  ; CHECK: bl end
+  call void @end()
+
+  ret void
+}