[AVR] Always expand STDSPQRr & STDWSPQRr

[Patryk27]

Summary

Currently, STDSPQRr and STDWSPQRr are expanded only during
AVRFrameLowering - this means that if any of those instructions happen
to appear _outside_ of the typical FrameSetup / FrameDestroy
context, they wouldn't get substituted, eventually leading to a crash:

LLVM ERROR: Not supported instr: <MCInst XXX <MCOperand Reg:1>
<MCOperand Imm:15> <MCOperand Reg:53>>

This commit fixes this issue by moving expansion of those two opcodes
into AVRExpandPseudo.

This bug was originally discovered due to the Rust compiler_builtins
library. Its 0.1.37 release contained a 128-bit software
division/remainder routine that exercised this buggy branch in the code.

[Patryk27]

cc @benshi001 🙂

So far I've just moved appropriate parts of code from the previous merge request (https://reviews.llvm.org/D114611), but I haven't yet applied 3.2 & 3.3 from https://reviews.llvm.org/D114611#3394757, since I don't quite understand those suggestions - currently the expansion uses either the Y _or_ the X register, and it looks like you'd rather for that instruction to always rely on X?

[Patryk27]

Rebase

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1221

Could you please add cases to show the use of R29R28 or R31R30 in different situations ?

llvm/test/CodeGen/AVR/stdwstk.ll

3

I perfer the .mir tests in your previous https://reviews.llvm.org/D114611.

It would be better to seperate to STDWSPQRr.mir and STDSPQRr.mir.

[benshi001]

llvm/lib/Target/AVR/AVRFrameLowering.cpp

369

I do not think the removal of fixStackStores at here is good, since we seperate the preparation of R31R30 and the substitution of STDWSPQRr-> STDWPtrQRr. It makes the code hard to understand, and I also concern there might be other issues introduced.

So I suggest we keep this fixStackStores call.

[benshi001]

This comment was removed by benshi001.

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1217

"SP is expected as base pointer"

1221

Sorry, ignore this comment.

llvm/lib/Target/AVR/AVRFrameLowering.cpp

369

I suggest a moderate way:

Keep the second fixStackStores (with R31R30) and remove the first one (R29R28). And in the expand<AVR::STDWSPQRr>, add an extra line

assert(STI.getFrameLowering()->hasReservedCallFrame(MF) &&
       "unexpected STDWSPQRr pseudo instruction");

llvm/test/CodeGen/AVR/stdwstk.ll

3

Sorry, igmore this comment.

8–10

Remove these lines

; See:
; - https://reviews.llvm.org/D114611
; - https://reviews.llvm.org/D95664

[benshi001]

llvm/lib/Target/AVR/AVRFrameLowering.cpp

369

Then we only substitute SP with R29R28.

[Patryk27]

Applying code review comments

[Patryk27]

Thanks for the review - I've applied all the changes :-)

llvm/lib/Target/AVR/AVRFrameLowering.cpp

369

Sure, that's a nice approach!

llvm/test/CodeGen/AVR/stdwstk.ll

3

I'd love to add those somehow, but since expanding those opcodes reads hasReservedCallFrame(), we'd have to make those two *.mir tests execute AVRFrameAnalyzer, which seems clumsy to implement & use.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

if (Imm >= 63) {
     MIBHI.setMemRefs(MI.memoperands());
   }
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
+bool AVRExpandPseudo::expand<AVR::STDSPQRr>(Block &MBB, BlockIt MBBI) {
+  MachineInstr &MI = *MBBI;
+  const MachineFunction &MF = *MBB.getParent();
+  const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
+
+  assert(MI.getOperand(0).getReg() == AVR::SP &&
+         "SP is expected as base pointer");
+
+  assert(STI.getFrameLowering()->hasReservedCallFrame(MF) &&
+         "unexpected STDSPQRr pseudo instruction");
+
+  MI.setDesc(TII->get(AVR::STDPtrQRr));
+  MI.getOperand(0).setReg(AVR::R29R28);
+
+  return true;
+}
+
+template <>
+bool AVRExpandPseudo::expand<AVR::STDWSPQRr>(Block &MBB, BlockIt MBBI) {
+  MachineInstr &MI = *MBBI;
+  const MachineFunction &MF = *MBB.getParent();
+  const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
+
+  assert(MI.getOperand(0).getReg() == AVR::SP &&
+         "SP is expected as base pointer");
+
+  assert(STI.getFrameLowering()->hasReservedCallFrame(MF) &&
+         "unexpected STDWSPQRr pseudo instruction");
+
+  MI.setDesc(TII->get(AVR::STDWPtrQRr));
+  MI.getOperand(0).setReg(AVR::R29R28);
+
+  return true;
+}
+
+template <>
 bool AVRExpandPseudo::expand<AVR::INWRdA>(Block &MBB, BlockIt MBBI) {
   MachineInstr &MI = *MBBI;
   Register DstLoReg, DstHiReg;
   unsigned Imm = MI.getOperand(1).getImm();
   Register DstReg = MI.getOperand(0).getReg();
   bool DstIsDead = MI.getOperand(0).isDead();
   unsigned OpLo = AVR::INRdA;
   unsigned OpHi = AVR::INRdA;
case AVR::LDDWRdYQ: //: FIXME: remove this once PR13375 gets fixed
     EXPAND(AVR::AtomicStore8);
     EXPAND(AVR::AtomicStore16);
     EXPAND(AVR::AtomicFence);
     EXPAND(AVR::STSWKRr);
     EXPAND(AVR::STWPtrRr);
     EXPAND(AVR::STWPtrPiRr);
     EXPAND(AVR::STWPtrPdRr);
     EXPAND(AVR::STDWPtrQRr);
+    EXPAND(AVR::STDSPQRr);
+    EXPAND(AVR::STDWSPQRr);
     EXPAND(AVR::INWRdA);
     EXPAND(AVR::OUTWARr);
     EXPAND(AVR::PUSHWRr);
     EXPAND(AVR::POPWRd);
     EXPAND(AVR::ROLBRd);
     EXPAND(AVR::RORBRd);
     EXPAND(AVR::LSLWRd);
     EXPAND(AVR::LSRWRd);

llvm/lib/Target/AVR/AVRFrameLowering.cpp

if (isUInt<6>(FrameSize)) {
       Opcode = AVR::ADIWRdK;
     } else {
       Opcode = AVR::SUBIWRdK;
       FrameSize = -FrameSize;
     }
 
     // Restore the frame pointer by doing FP += <size>.
     MachineInstr *MI = BuildMI(MBB, MBBI, DL, TII.get(Opcode), AVR::R29R28)
                            .addReg(AVR::R29R28, RegState::Kill)
                            .addImm(FrameSize);
     // The SREG implicit def is dead.
     MI->getOperand(3).setIsDead();
   }
 
   // Write back R29R28 to SP and temporarily disable interrupts.
   BuildMI(MBB, MBBI, DL, TII.get(AVR::SPWRITE), AVR::SP)
       .addReg(AVR::R29R28, RegState::Kill);
 
bool AVRFrameLowering::restoreCalleeSavedRegisters(
 
   return true;
 }
 
 /// Replace pseudo store instructions that pass arguments through the stack with
 /// real instructions.
 static void fixStackStores(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator StartMI,
-                           const TargetInstrInfo &TII, Register FP) {
+                           const TargetInstrInfo &TII) {
   // Iterate through the BB until we hit a call instruction or we reach the end.
   for (MachineInstr &MI :
        llvm::make_early_inc_range(llvm::make_range(StartMI, MBB.end()))) {
     if (MI.isCall())
       break;
 
     unsigned Opcode = MI.getOpcode();
 
     // Only care of pseudo store instructions where SP is the base pointer.
     if (Opcode != AVR::STDSPQRr && Opcode != AVR::STDWSPQRr)
       continue;
 
     assert(MI.getOperand(0).getReg() == AVR::SP &&
-           "Invalid register, should be SP!");
+           "SP is expected as base pointer");
 
     // Replace this instruction with a regular store. Use Y as the base
     // pointer since it is guaranteed to contain a copy of SP.
     unsigned STOpc =
         (Opcode == AVR::STDWSPQRr) ? AVR::STDWPtrQRr : AVR::STDPtrQRr;
 
     MI.setDesc(TII.get(STOpc));
-    MI.getOperand(0).setReg(FP);
+    MI.getOperand(0).setReg(AVR::R31R30);
   }
 }
 
 MachineBasicBlock::iterator AVRFrameLowering::eliminateCallFramePseudoInstr(
     MachineFunction &MF, MachineBasicBlock &MBB,
     MachineBasicBlock::iterator MI) const {
   const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
   const AVRInstrInfo &TII = *STI.getInstrInfo();
 
-  // There is nothing to insert when the call frame memory is allocated during
-  // function entry. Delete the call frame pseudo and replace all pseudo stores
-  // with real store instructions.
   if (hasReservedCallFrame(MF)) {
-    fixStackStores(MBB, MI, TII, AVR::R29R28);
     return MBB.erase(MI);
   }
 
   DebugLoc DL = MI->getDebugLoc();
   unsigned int Opcode = MI->getOpcode();
   int Amount = TII.getFrameSize(*MI);
 
-  // ADJCALLSTACKUP and ADJCALLSTACKDOWN are converted to adiw/subi
-  // instructions to read and write the stack pointer in I/O space.
-  if (Amount != 0) {
-    assert(getStackAlign() == Align(1) && "Unsupported stack alignment");
+  if (Amount == 0) {
+    return MBB.erase(MI);
+  }
+
 
-    if (Opcode == TII.getCallFrameSetupOpcode()) {
-      // Update the stack pointer.
-      // In many cases this can be done far more efficiently by pushing the
-      // relevant values directly to the stack. However, doing that correctly
-      // (in the right order, possibly skipping some empty space for undef
-      // values, etc) is tricky and thus left to be optimized in the future.
-      BuildMI(MBB, MI, DL, TII.get(AVR::SPREAD), AVR::R31R30).addReg(AVR::SP);
 
-      MachineInstr *New =
-          BuildMI(MBB, MI, DL, TII.get(AVR::SUBIWRdK), AVR::R31R30)
-              .addReg(AVR::R31R30, RegState::Kill)
-              .addImm(Amount);
-      New->getOperand(3).setIsDead();
 
-      BuildMI(MBB, MI, DL, TII.get(AVR::SPWRITE), AVR::SP).addReg(AVR::R31R30);
 
-      // Make sure the remaining stack stores are converted to real store
-      // instructions.
-      fixStackStores(MBB, MI, TII, AVR::R31R30);
-    } else {
-      assert(Opcode == TII.getCallFrameDestroyOpcode());
+    fixStackStores(MBB, MI, TII);
 
-      // Note that small stack changes could be implemented more efficiently
-      // with a few pop instructions instead of the 8-9 instructions now
-      // required.
 
-      // Select the best opcode to adjust SP based on the offset size.
-      unsigned addOpcode;
-      if (isUInt<6>(Amount)) {
-        addOpcode = AVR::ADIWRdK;
-      } else {
-        addOpcode = AVR::SUBIWRdK;
-        Amount = -Amount;
-      }
+    unsigned AddOpcode;
+
+      AddOpcode = AVR::ADIWRdK;
+      AddOpcode = AVR::SUBIWRdK;
 
-      // Build the instruction sequence.
-      BuildMI(MBB, MI, DL, TII.get(AVR::SPREAD), AVR::R31R30).addReg(AVR::SP);
 
-      MachineInstr *New = BuildMI(MBB, MI, DL, TII.get(addOpcode), AVR::R31R30)
-                              .addReg(AVR::R31R30, RegState::Kill)
-                              .addImm(Amount);
-      New->getOperand(3).setIsDead();
+    MachineInstr *New = BuildMI(MBB, MI, DL, TII.get(AddOpcode), AVR::R31R30)
 
-      BuildMI(MBB, MI, DL, TII.get(AVR::SPWRITE), AVR::SP)
-          .addReg(AVR::R31R30, RegState::Kill);
-    }
-  }
 
   return MBB.erase(MI);
 }
 
 void AVRFrameLowering::determineCalleeSaves(MachineFunction &MF,
                                             BitVector &SavedRegs,
                                             RegScavenger *RS) const {
   TargetFrameLowering::determineCalleeSaves(MF, SavedRegs, RS);

 /// Scans the function for allocas and used arguments
 /// that are passed through the stack.
 struct AVRFrameAnalyzer : public MachineFunctionPass {
   static char ID;
   AVRFrameAnalyzer() : MachineFunctionPass(ID) {}
 
   bool runOnMachineFunction(MachineFunction &MF) override {
     const MachineFrameInfo &MFI = MF.getFrameInfo();
-    AVRMachineFunctionInfo *FuncInfo = MF.getInfo<AVRMachineFunctionInfo>();
+    AVRMachineFunctionInfo *AFI = MF.getInfo<AVRMachineFunctionInfo>();
 
     // If there are no fixed frame indexes during this stage it means there
     // are allocas present in the function.
     if (MFI.getNumObjects() != MFI.getNumFixedObjects()) {
       // Check for the type of allocas present in the function. We only care
       // about fixed size allocas so do not give false positives if only
       // variable sized allocas are present.
       for (unsigned i = 0, e = MFI.getObjectIndexEnd(); i != e; ++i) {
         // Variable sized objects have size 0.
         if (MFI.getObjectSize(i)) {
-          FuncInfo->setHasAllocas(true);
+          AFI->setHasAllocas(true);
           break;
         }
       }
     }
 
     // If there are fixed frame indexes present, scan the function to see if
     // they are really being used.
     if (MFI.getNumFixedObjects() == 0) {
for (const MachineBasicBlock &BB : MF) {
         }
 
         for (const MachineOperand &MO : MI.operands()) {
           if (!MO.isFI()) {
             continue;
           }
 
           if (MFI.isFixedObjectIndex(MO.getIndex())) {
-            FuncInfo->setHasStackArgs(true);
+            AFI->setHasStackArgs(true);
             return false;
           }
         }
       }
     }
 
     return false;
   }

llvm/test/CodeGen/AVR/stdwstk.ll

+; RUN: llc < %s -march=avr -mcpu=atmega328 -O1 | FileCheck %s
+; CHECK-NOT: stdwstk
+
+; Checks that we expand STDWSPQRr always - even if it appears outside of the
+; FrameSetup/FrameDestroy context.
+
+declare { } @foo(i128, i128) addrspace(1)
+
+define i128 @bar(i128 %a, i128 %b) addrspace(1) {
+  %b_neg = icmp slt i128 %b, 0
+  %divisor = select i1 %b_neg, i128 0, i128 %b
+  %result = tail call fastcc addrspace(1) { } @foo(i128 undef, i128 %divisor)
+
+  ret i128 0
+}