X86WinAllocaExpander: Drop code looking through register copies (PR41786)

[hans]

Summary

This code was never covered by tests, in PR41786 it was pointed out that the deletion part doesn't work, and in a full Chrome build I was never able to hit the code path that looks through copies. It seems the situation it's supposed to handle doesn't actually come up in practice.

Delete it to simplify the code.

[RKSimon]

llvm/lib/Target/X86/X86WinAllocaExpander.cpp

269 ↗

(On Diff #198613)

Can this be null? We were testing for it before and still do above.

[hans]

llvm/lib/Target/X86/X86WinAllocaExpander.cpp

269 ↗

(On Diff #198613)

Yes, we should keep that. Good catch.

[RKSimon]

One last comment from me - @rnk please can you take a look? I know very little about this code....

llvm/lib/Target/X86/X86WinAllocaExpander.cpp

272 ↗

(On Diff #198624)

(style) drop the braces

[rnk]

lgtm, too

llvm/lib/Target/X86/X86WinAllocaExpander.cpp

91–94 ↗

(On Diff #198624)

As a follow-up, I think it would be reasonable for WIN_ALLOCA_* to accept either reg or imm operands so we won't need this code at all. Right now we emit this MI:

%12:gr32 = MOV32ri 12
WIN_ALLOCA_32 %12:gr32, implicit-def dead $eax, implicit-def dead $esp, implicit-def dead $eflags, implicit $esp

But it's not really necessary, since in the end we're going to make SUB64ri instructions with the immediate.

I think the instruction wasn't designed this way because most truly dynamic allocas do not use a fixed size. It's only inalloca that introduces them.

llvm/trunk/lib/Target/X86/X86WinAllocaExpander.cpp

 static int64_t getWinAllocaAmount(MachineInstr *MI, MachineRegisterInfo *MRI) {
   assert(MI->getOpcode() == X86::WIN_ALLOCA_32 ||
          MI->getOpcode() == X86::WIN_ALLOCA_64);
   assert(MI->getOperand(0).isReg());
 
   unsigned AmountReg = MI->getOperand(0).getReg();
   MachineInstr *Def = MRI->getUniqueVRegDef(AmountReg);
 
-  // Look through copies.
-  while (Def && Def->isCopy() && Def->getOperand(1).isReg())
-    Def = MRI->getUniqueVRegDef(Def->getOperand(1).getReg());
-
   if (!Def ||
       (Def->getOpcode() != X86::MOV32ri && Def->getOpcode() != X86::MOV64ri) ||
       !Def->getOperand(1).isImm())
     return -1;
 
   return Def->getOperand(1).getImm();
 }
 
if (!NoStackArgProbe) {
           .addReg(MI->getOperand(0).getReg());
     }
     break;
   }
 
   unsigned AmountReg = MI->getOperand(0).getReg();
   MI->eraseFromParent();
 
-  // Delete the definition of AmountReg, possibly walking a chain of copies.
-  for (;;) {
-    if (!MRI->use_empty(AmountReg))
-      break;
-    MachineInstr *AmountDef = MRI->getUniqueVRegDef(AmountReg);
-    if (!AmountDef)
-      break;
-    if (AmountDef->isCopy() && AmountDef->getOperand(1).isReg())
-      AmountReg = AmountDef->getOperand(1).isReg();
+  // Delete the definition of AmountReg.
+  if (MRI->use_empty(AmountReg))
+    if (MachineInstr *AmountDef = MRI->getUniqueVRegDef(AmountReg))
       AmountDef->eraseFromParent();
-    break;
-  }
 }
 
 bool X86WinAllocaExpander::runOnMachineFunction(MachineFunction &MF) {
   if (!MF.getInfo<X86MachineFunctionInfo>()->hasWinAlloca())
     return false;
 
   MRI = &MF.getRegInfo();
   STI = &MF.getSubtarget<X86Subtarget>();