[AVR] Fix atomicrmw result value

[aykevl]

Summary

This patch fixes the atomicrmw result value to be the value before the operation instead of the value after the operation. This was a bug, left as a FIXME in the code (see D97127).

From the LangRef:

The contents of memory at the location specified by the ‘<pointer>’ operand are atomically read, modified, and written back. The original value at the location is returned.

Doing this expansion early allows the register allocator to arrange registers in such a way that commutable operations are simply swapped around as needed, which results in shorter code while still being correct.

[benshi001]

llvm/lib/Target/AVR/AVRISelLowering.cpp

1705

Also, do we need to make a helper function int getRegScratch(void) in AVRSubtarget.h, just like I have done as getIORegRAMPZ(void) ? The new helper function will return r16 for avr-tiny devices.

Also, do we need a helper function int getIORegSREG(void) instead of hard coded 0x3f ?

[benshi001]

llvm/lib/Target/AVR/AVRISelLowering.cpp

1733

Do we need a flag RegState::Define for Result ?

[aykevl]

• use getIORegSREG() instead of hardcoding I/O address 0x3f.

[aykevl]

llvm/lib/Target/AVR/AVRISelLowering.cpp

1705

See my comment in D117426.

1733

No, because BuildMI will always set RegState::Define for the last parameter (Result in this case).

[aykevl]

llvm/lib/Target/AVR/AVRISelLowering.cpp

1705

Sorry I mean D117425.

[benshi001]

llvm/test/CodeGen/AVR/atomics/load16.ll

32

I am OK with this patch. Just a bit confusion.

In the next test file load8.ll, RR1 is the address and RD is the loaded value.

ld [[RD:r[0-9]+]], [[RR:(X|Y|Z)]]

I suggest you make them in accordance with each other when committing.

[aykevl]

• small update to the load8.ll test to make it more readable

[aykevl]

llvm/test/CodeGen/AVR/atomics/load16.ll

32

Sounds reasonable. However, I would prefer to do this in a separate patch so that the diff in this patch is smaller. Here it is: D117991
If you accept that patch I can commit them after each other.

[benshi001]

llvm/test/CodeGen/AVR/atomics/load16.ll

32

OK. Please commit them. :)

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

private:
 
   const AVRRegisterInfo *TRI;
   const TargetInstrInfo *TII;
 
   /// The register to be used for temporary storage.
   const Register SCRATCH_REGISTER = AVR::R0;
   /// The register that will always contain zero.
   const Register ZERO_REGISTER = AVR::R1;
-  /// The IO address of the status register.
-  const unsigned SREG_ADDR = 0x3f;
 
   bool expandMBB(Block &MBB);
   bool expandMI(Block &MBB, BlockIt MBBI);
   template <unsigned OP> bool expand(Block &MBB, BlockIt MBBI);
 
   MachineInstrBuilder buildMI(Block &MBB, BlockIt MBBI, unsigned Opcode) {
     return BuildMI(MBB, MBBI, MBBI->getDebugLoc(), TII->get(Opcode));
   }
private:
 
   template <typename Func> bool expandAtomic(Block &MBB, BlockIt MBBI, Func f);
 
   template <typename Func>
   bool expandAtomicBinaryOp(unsigned Opcode, Block &MBB, BlockIt MBBI, Func f);
 
   bool expandAtomicBinaryOp(unsigned Opcode, Block &MBB, BlockIt MBBI);
 
-  bool expandAtomicArithmeticOp(unsigned MemOpcode, unsigned ArithOpcode,
-                                Block &MBB, BlockIt MBBI);
-
   /// Specific shift implementation.
   bool expandLSLB7Rd(Block &MBB, BlockIt MBBI);
   bool expandLSRB7Rd(Block &MBB, BlockIt MBBI);
   bool expandASRB6Rd(Block &MBB, BlockIt MBBI);
   bool expandASRB7Rd(Block &MBB, BlockIt MBBI);
   bool expandLSLW4Rd(Block &MBB, BlockIt MBBI);
   bool expandLSRW4Rd(Block &MBB, BlockIt MBBI);
   bool expandLSLW8Rd(Block &MBB, BlockIt MBBI);

 
 template <>
 bool AVRExpandPseudo::expand<AVR::ELPMWRdZPi>(Block &MBB, BlockIt MBBI) {
   llvm_unreachable("16-bit ELPMPi is unimplemented");
 }
 
 template <typename Func>
 bool AVRExpandPseudo::expandAtomic(Block &MBB, BlockIt MBBI, Func f) {
-  // Remove the pseudo instruction.
   MachineInstr &MI = *MBBI;
+  const AVRSubtarget &STI = MBB.getParent()->getSubtarget<AVRSubtarget>();
 
   // Store the SREG.
   buildMI(MBB, MBBI, AVR::INRdA)
       .addReg(SCRATCH_REGISTER, RegState::Define)
-      .addImm(SREG_ADDR);
+      .addImm(STI.getIORegSREG());
 
   // Disable exceptions.
   buildMI(MBB, MBBI, AVR::BCLRs).addImm(7); // CLI
 
   f(MI);
 
   // Restore the status reg.
-  buildMI(MBB, MBBI, AVR::OUTARr).addImm(SREG_ADDR).addReg(SCRATCH_REGISTER);
+  buildMI(MBB, MBBI, AVR::OUTARr)
+      .addImm(STI.getIORegSREG())
+      .addReg(SCRATCH_REGISTER);
 
   MI.eraseFromParent();
   return true;
 }
 
 template <typename Func>
 bool AVRExpandPseudo::expandAtomicBinaryOp(unsigned Opcode, Block &MBB,
                                            BlockIt MBBI, Func f) {
   return expandAtomic(MBB, MBBI, [&](MachineInstr &MI) {
     auto Op1 = MI.getOperand(0);
     auto Op2 = MI.getOperand(1);
 
     MachineInstr &NewInst =
         *buildMI(MBB, MBBI, Opcode).add(Op1).add(Op2).getInstr();
     f(NewInst);
   });
 }
 
 bool AVRExpandPseudo::expandAtomicBinaryOp(unsigned Opcode, Block &MBB,
                                            BlockIt MBBI) {
   return expandAtomicBinaryOp(Opcode, MBB, MBBI, [](MachineInstr &MI) {});
 }
 
-bool AVRExpandPseudo::expandAtomicArithmeticOp(unsigned Width,
-                                               unsigned ArithOpcode, Block &MBB,
-                                               BlockIt MBBI) {
-  return expandAtomic(MBB, MBBI, [&](MachineInstr &MI) {
-    auto DstReg = MI.getOperand(0).getReg();
-    auto PtrOp = MI.getOperand(1);
-    auto SrcReg = MI.getOperand(2).getReg();
-
-    unsigned LoadOpcode = (Width == 8) ? AVR::LDRdPtr : AVR::LDWRdPtr;
-    unsigned StoreOpcode = (Width == 8) ? AVR::STPtrRr : AVR::STWPtrRr;
-
-    // FIXME: this returns the new value (after the operation), not the old
-    // value as the atomicrmw instruction is supposed to do!
-
-    // Create the load
-    buildMI(MBB, MBBI, LoadOpcode, DstReg).addReg(PtrOp.getReg());
-
-    // Create the arithmetic op
-    buildMI(MBB, MBBI, ArithOpcode, DstReg).addReg(DstReg).addReg(SrcReg);
-
-    // Create the store
-    buildMI(MBB, MBBI, StoreOpcode).add(PtrOp).addReg(DstReg);
-  });
-}
-
 Register AVRExpandPseudo::scavengeGPR8(MachineInstr &MI) {
   MachineBasicBlock &MBB = *MI.getParent();
   RegScavenger RS;
 
   RS.enterBasicBlock(MBB);
   RS.forward(MI);
 
   BitVector Candidates =

 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::AtomicStore16>(Block &MBB, BlockIt MBBI) {
   return expandAtomicBinaryOp(AVR::STWPtrRr, MBB, MBBI);
 }
 
 template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadAdd8>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(8, AVR::ADDRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadAdd16>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(16, AVR::ADDWRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadSub8>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(8, AVR::SUBRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadSub16>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(16, AVR::SUBWRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadAnd8>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(8, AVR::ANDRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadAnd16>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(16, AVR::ANDWRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadOr8>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(8, AVR::ORRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadOr16>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(16, AVR::ORWRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadXor8>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(8, AVR::EORRdRr, MBB, MBBI);
-}
-
-template <>
-bool AVRExpandPseudo::expand<AVR::AtomicLoadXor16>(Block &MBB, BlockIt MBBI) {
-  return expandAtomicArithmeticOp(16, AVR::EORWRdRr, MBB, MBBI);
-}
-
-template <>
 bool AVRExpandPseudo::expand<AVR::AtomicFence>(Block &MBB, BlockIt MBBI) {
   // On AVR, there is only one core and so atomic fences do nothing.
   MBBI->eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::STSWKRr>(Block &MBB, BlockIt MBBI) {
buildMI(MBB, MBBI, OpHi)
       .setMIFlags(Flags);
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::SPWRITE>(Block &MBB, BlockIt MBBI) {
+  const AVRSubtarget &STI = MBB.getParent()->getSubtarget<AVRSubtarget>();
   MachineInstr &MI = *MBBI;
   Register SrcLoReg, SrcHiReg;
   Register SrcReg = MI.getOperand(1).getReg();
   bool SrcIsKill = MI.getOperand(1).isKill();
   unsigned Flags = MI.getFlags();
   TRI->splitReg(SrcReg, SrcLoReg, SrcHiReg);
 
   buildMI(MBB, MBBI, AVR::INRdA)
       .addReg(AVR::R0, RegState::Define)
-      .addImm(SREG_ADDR)
+      .addImm(STI.getIORegSREG())
       .setMIFlags(Flags);
 
   buildMI(MBB, MBBI, AVR::BCLRs).addImm(0x07).setMIFlags(Flags);
 
   buildMI(MBB, MBBI, AVR::OUTARr)
       .addImm(0x3e)
       .addReg(SrcHiReg, getKillRegState(SrcIsKill))
       .setMIFlags(Flags);
 
   buildMI(MBB, MBBI, AVR::OUTARr)
-      .addImm(SREG_ADDR)
+      .addImm(STI.getIORegSREG())
       .addReg(AVR::R0, RegState::Kill)
       .setMIFlags(Flags);
 
   buildMI(MBB, MBBI, AVR::OUTARr)
       .addImm(0x3d)
       .addReg(SrcLoReg, getKillRegState(SrcIsKill))
       .setMIFlags(Flags);
 
case AVR::LDDWRdYQ: //: FIXME: remove this once PR13375 gets fixed
     EXPAND(AVR::ELPMBRdZ);
     EXPAND(AVR::ELPMWRdZ);
     EXPAND(AVR::ELPMBRdZPi);
     EXPAND(AVR::ELPMWRdZPi);
     EXPAND(AVR::AtomicLoad8);
     EXPAND(AVR::AtomicLoad16);
     EXPAND(AVR::AtomicStore8);
     EXPAND(AVR::AtomicStore16);
-    EXPAND(AVR::AtomicLoadAdd8);
-    EXPAND(AVR::AtomicLoadAdd16);
-    EXPAND(AVR::AtomicLoadSub8);
-    EXPAND(AVR::AtomicLoadSub16);
-    EXPAND(AVR::AtomicLoadAnd8);
-    EXPAND(AVR::AtomicLoadAnd16);
-    EXPAND(AVR::AtomicLoadOr8);
-    EXPAND(AVR::AtomicLoadOr16);
-    EXPAND(AVR::AtomicLoadXor8);
-    EXPAND(AVR::AtomicLoadXor16);
     EXPAND(AVR::AtomicFence);
     EXPAND(AVR::STSWKRr);
     EXPAND(AVR::STWPtrRr);
     EXPAND(AVR::STWPtrPiRr);
     EXPAND(AVR::STWPtrPdRr);
     EXPAND(AVR::STDWPtrQRr);
     EXPAND(AVR::INWRdA);
     EXPAND(AVR::OUTWARr);

llvm/lib/Target/AVR/AVRFrameLowering.cpp

void AVRFrameLowering::emitPrologue(MachineFunction &MF,
   // Emit special prologue code to save R1, R0 and SREG in interrupt/signal
   // handlers before saving any other registers.
   if (AFI->isInterruptOrSignalHandler()) {
     BuildMI(MBB, MBBI, DL, TII.get(AVR::PUSHWRr))
         .addReg(AVR::R1R0, RegState::Kill)
         .setMIFlag(MachineInstr::FrameSetup);
 
     BuildMI(MBB, MBBI, DL, TII.get(AVR::INRdA), AVR::R0)
-        .addImm(0x3f)
+        .addImm(STI.getIORegSREG())
         .setMIFlag(MachineInstr::FrameSetup);
     BuildMI(MBB, MBBI, DL, TII.get(AVR::PUSHRr))
         .addReg(AVR::R0, RegState::Kill)
         .setMIFlag(MachineInstr::FrameSetup);
     BuildMI(MBB, MBBI, DL, TII.get(AVR::EORRdRr))
         .addReg(AVR::R0, RegState::Define)
         .addReg(AVR::R0, RegState::Kill)
         .addReg(AVR::R0, RegState::Kill)
static void restoreStatusRegister(MachineFunction &MF, MachineBasicBlock &MBB) {
   const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
   const AVRInstrInfo &TII = *STI.getInstrInfo();
 
   // Emit special epilogue code to restore R1, R0 and SREG in interrupt/signal
   // handlers at the very end of the function, just before reti.
   if (AFI->isInterruptOrSignalHandler()) {
     BuildMI(MBB, MBBI, DL, TII.get(AVR::POPRd), AVR::R0);
     BuildMI(MBB, MBBI, DL, TII.get(AVR::OUTARr))
-        .addImm(0x3f)
+        .addImm(STI.getIORegSREG())
         .addReg(AVR::R0, RegState::Kill);
     BuildMI(MBB, MBBI, DL, TII.get(AVR::POPWRd), AVR::R1R0);
   }
 }
 
 void AVRFrameLowering::emitEpilogue(MachineFunction &MF,
                                     MachineBasicBlock &MBB) const {
   const AVRMachineFunctionInfo *AFI = MF.getInfo<AVRMachineFunctionInfo>();

llvm/lib/Target/AVR/AVRISelLowering.h

SDValue LowerCallResult(SDValue Chain, SDValue InFlag,
                           SmallVectorImpl<SDValue> &InVals) const;
 
 protected:
   const AVRSubtarget &Subtarget;
 
 private:
   MachineBasicBlock *insertShift(MachineInstr &MI, MachineBasicBlock *BB) const;
   MachineBasicBlock *insertMul(MachineInstr &MI, MachineBasicBlock *BB) const;
+  MachineBasicBlock *insertAtomicArithmeticOp(MachineInstr &MI,
+                                              MachineBasicBlock *BB,
+                                              unsigned Opcode, int Width) const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_AVR_ISEL_LOWERING_H

llvm/lib/Target/AVR/AVRISelLowering.cpp

MachineBasicBlock *AVRTargetLowering::insertMul(MachineInstr &MI,
   if (isCopyMulResult(I))
     ++I;
   BuildMI(*BB, I, MI.getDebugLoc(), TII.get(AVR::EORRdRr), AVR::R1)
       .addReg(AVR::R1)
       .addReg(AVR::R1);
   return BB;
 }
 
+// Lower atomicrmw operation to disable interrupts, do operation, and restore
+// interrupts. This works because all AVR microcontrollers are single core.
+MachineBasicBlock *AVRTargetLowering::insertAtomicArithmeticOp(
+    MachineInstr &MI, MachineBasicBlock *BB, unsigned Opcode, int Width) const {
+  MachineRegisterInfo &MRI = BB->getParent()->getRegInfo();
+  const TargetInstrInfo &TII = *Subtarget.getInstrInfo();
+  MachineBasicBlock::iterator I(MI);
+  const Register SCRATCH_REGISTER = AVR::R0;
+  DebugLoc dl = MI.getDebugLoc();
+
+  // Example instruction sequence, for an atomic 8-bit add:
+  //   ldi r25, 5
+  //   in r0, SREG
+  //   cli
+  //   ld r24, X
+  //   add r25, r24
+  //   st X, r25
+  //   out SREG, r0
+
+  const TargetRegisterClass *RC =
+      (Width == 8) ? &AVR::GPR8RegClass : &AVR::DREGSRegClass;
+  unsigned LoadOpcode = (Width == 8) ? AVR::LDRdPtr : AVR::LDWRdPtr;
+  unsigned StoreOpcode = (Width == 8) ? AVR::STPtrRr : AVR::STWPtrRr;
+
+  // Disable interrupts.
+  BuildMI(*BB, I, dl, TII.get(AVR::INRdA), SCRATCH_REGISTER)
+      .addImm(Subtarget.getIORegSREG());
+  BuildMI(*BB, I, dl, TII.get(AVR::BCLRs)).addImm(7);
+
+  // Load the original value.
+  BuildMI(*BB, I, dl, TII.get(LoadOpcode), MI.getOperand(0).getReg())
+      .add(MI.getOperand(1));
+
+  // Do the arithmetic operation.
+  Register Result = MRI.createVirtualRegister(RC);
+  BuildMI(*BB, I, dl, TII.get(Opcode), Result)
+      .addReg(MI.getOperand(0).getReg())
+      .add(MI.getOperand(2));
+
+  // Store the result.
+  BuildMI(*BB, I, dl, TII.get(StoreOpcode))
+      .add(MI.getOperand(1))
+      .addReg(Result);
+
+  // Restore interrupts.
+  BuildMI(*BB, I, dl, TII.get(AVR::OUTARr))
+      .addImm(Subtarget.getIORegSREG())
+      .addReg(SCRATCH_REGISTER);
+
+  // Remove the pseudo instruction.
+  MI.eraseFromParent();
+  return BB;
+}
+
 MachineBasicBlock *
 AVRTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
                                                MachineBasicBlock *MBB) const {
   int Opc = MI.getOpcode();
 
   // Pseudo shift instructions with a non constant shift amount are expanded
   // into a loop.
   switch (Opc) {
   case AVR::Lsl8:
   case AVR::Lsl16:
   case AVR::Lsr8:
   case AVR::Lsr16:
   case AVR::Rol8:
   case AVR::Rol16:
   case AVR::Ror8:
   case AVR::Ror16:
   case AVR::Asr8:
   case AVR::Asr16:
     return insertShift(MI, MBB);
   case AVR::MULRdRr:
   case AVR::MULSRdRr:
     return insertMul(MI, MBB);
+  case AVR::AtomicLoadAdd8:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::ADDRdRr, 8);
+  case AVR::AtomicLoadAdd16:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::ADDWRdRr, 16);
+  case AVR::AtomicLoadSub8:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::SUBRdRr, 8);
+  case AVR::AtomicLoadSub16:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::SUBWRdRr, 16);
+  case AVR::AtomicLoadAnd8:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::ANDRdRr, 8);
+  case AVR::AtomicLoadAnd16:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::ANDWRdRr, 16);
+  case AVR::AtomicLoadOr8:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::ORRdRr, 8);
+  case AVR::AtomicLoadOr16:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::ORWRdRr, 16);
+  case AVR::AtomicLoadXor8:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::EORRdRr, 8);
+  case AVR::AtomicLoadXor16:
+    return insertAtomicArithmeticOp(MI, MBB, AVR::EORWRdRr, 16);
   }
 
   assert((Opc == AVR::Select16 || Opc == AVR::Select8) &&
          "Unexpected instr type to insert");
 
   const AVRInstrInfo &TII = (const AVRInstrInfo &)*MI.getParent()
                                 ->getParent()
                                 ->getSubtarget()

llvm/lib/Target/AVR/AVRInstrInfo.td

class AtomicStore<PatFrag Op, RegisterClass DRC, RegisterClass PTRRC>
     : Pseudo<(outs),
              (ins PTRRC
               : $rd, DRC
               : $rr),
              "atomic_op", [(Op i16
                             : $rd, DRC
                             : $rr)]>;
 
-let Constraints =
-    "@earlyclobber $rd" in class AtomicLoadOp<PatFrag Op, RegisterClass DRC,
-                                              RegisterClass PTRRC>
-    : Pseudo<(outs DRC
-              : $rd),
-             (ins PTRRC
-              : $rr, DRC
-              : $operand),
-             "atomic_op", [(set DRC
-                            : $rd, (Op i16
-                                    : $rr, DRC
-                                    : $operand))]>;
-
-// FIXME: I think 16-bit atomic binary ops need to mark
-// r0 as clobbered.
+class AtomicLoadOp<PatFrag Op, RegisterClass DRC, RegisterClass PTRRC>
+    : Pseudo<(outs DRC:$rd),
+             (ins PTRRC:$rr, DRC:$operand),
+             "atomic_op", [(set DRC:$rd, (Op i16:$rr, DRC:$operand))]>;
 
 // Atomic instructions
 // ===================
 //
-// These are all expanded by AVRExpandPseudoInsts
-//
 // 8-bit operations can use any pointer register because
 // they are expanded directly into an LD/ST instruction.
 //
 // 16-bit operations use 16-bit load/store postincrement instructions,
 // which require PTRDISPREGS.
 
 def AtomicLoad8 : AtomicLoad<atomic_load_8, GPR8, PTRREGS>;
 def AtomicLoad16 : AtomicLoad<atomic_load_16, DREGS, PTRDISPREGS>;
 
 def AtomicStore8 : AtomicStore<atomic_store_8, GPR8, PTRREGS>;
 def AtomicStore16 : AtomicStore<atomic_store_16, DREGS, PTRDISPREGS>;
 
 class AtomicLoadOp8<PatFrag Op> : AtomicLoadOp<Op, GPR8, PTRREGS>;
 class AtomicLoadOp16<PatFrag Op> : AtomicLoadOp<Op, DREGS, PTRDISPREGS>;
 
+let usesCustomInserter=1 in {
   def AtomicLoadAdd8 : AtomicLoadOp8<atomic_load_add_8>;
   def AtomicLoadAdd16 : AtomicLoadOp16<atomic_load_add_16>;
   def AtomicLoadSub8 : AtomicLoadOp8<atomic_load_sub_8>;
   def AtomicLoadSub16 : AtomicLoadOp16<atomic_load_sub_16>;
   def AtomicLoadAnd8 : AtomicLoadOp8<atomic_load_and_8>;
   def AtomicLoadAnd16 : AtomicLoadOp16<atomic_load_and_16>;
   def AtomicLoadOr8 : AtomicLoadOp8<atomic_load_or_8>;
   def AtomicLoadOr16 : AtomicLoadOp16<atomic_load_or_16>;
   def AtomicLoadXor8 : AtomicLoadOp8<atomic_load_xor_8>;
   def AtomicLoadXor16 : AtomicLoadOp16<atomic_load_xor_16>;
+}
 def AtomicFence
     : Pseudo<(outs), (ins), "atomic_fence", [(atomic_fence timm, timm)]>;
 
 // Indirect store from register to data space.
 def STSKRr : F32DM<0b1, (outs),
                    (ins imm16
                     : $k, GPR8
                     : $rd),

 def ROL : InstAlias<"rol\t$rd", (ADCRdRr GPR8 : $rd, GPR8 : $rd)>;
 
 // SER Rd
 // Alias for LDI Rd, 0xff
 // ---------
 // Sets all bits in a register.
 def : InstAlias<"ser\t$rd", (LDIRdK LD8 : $rd, 0xff), 0>;
 
-let Defs = [SREG] in def BSETs : FS<0, (outs),
-                                    (ins i8imm
-                                     : $s),
+let hasSideEffects=1 in {
+  let Defs = [SREG] in def BSETs : FS<0,
+                                      (outs),
+                                      (ins i8imm:$s),
                                       "bset\t$s", []>;
 
-let Defs = [SREG] in def BCLRs : FS<1, (outs),
-                                    (ins i8imm
-                                     : $s),
+  let Defs = [SREG] in def BCLRs : FS<1,
+                                      (outs),
+                                      (ins i8imm:$s),
                                       "bclr\t$s", []>;
+}
 
 // Set/clear aliases for the carry (C) status flag (bit 0).
 def : InstAlias<"sec", (BSETs 0)>;
 def : InstAlias<"clc", (BCLRs 0)>;
 
 // Set/clear aliases for the zero (Z) status flag (bit 1).
 def : InstAlias<"sez", (BSETs 1)>;
 def : InstAlias<"clz", (BCLRs 1)>;

llvm/lib/Target/AVR/AVRRegisterInfo.cpp

void AVRRegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
   MachineInstr &MI = *II;
   DebugLoc dl = MI.getDebugLoc();
   MachineBasicBlock &MBB = *MI.getParent();
   const MachineFunction &MF = *MBB.getParent();
   const AVRTargetMachine &TM = (const AVRTargetMachine &)MF.getTarget();
   const TargetInstrInfo &TII = *TM.getSubtargetImpl()->getInstrInfo();
   const MachineFrameInfo &MFI = MF.getFrameInfo();
   const TargetFrameLowering *TFI = TM.getSubtargetImpl()->getFrameLowering();
+  const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
   int FrameIndex = MI.getOperand(FIOperandNum).getIndex();
   int Offset = MFI.getObjectOffset(FrameIndex);
 
   // Add one to the offset because SP points to an empty slot.
   Offset += MFI.getStackSize() - TFI->getOffsetOfLocalArea() + 1;
   // Fold incoming offset.
   Offset += MI.getOperand(FIOperandNum + 1).getImm();
 
if ((Offset - 63 + 1) > 63) {
       SubOpc = AVR::SUBIWRdK;
       AddOffset = -AddOffset;
     }
 
     // It is possible that the spiller places this frame instruction in between
     // a compare and branch, invalidating the contents of SREG set by the
     // compare instruction because of the add/sub pairs. Conservatively save and
     // restore SREG before and after each add/sub pair.
-    BuildMI(MBB, II, dl, TII.get(AVR::INRdA), AVR::R0).addImm(0x3f);
+    BuildMI(MBB, II, dl, TII.get(AVR::INRdA), AVR::R0)
+        .addImm(STI.getIORegSREG());
 
     MachineInstr *New = BuildMI(MBB, II, dl, TII.get(AddOpc), AVR::R29R28)
                             .addReg(AVR::R29R28, RegState::Kill)
                             .addImm(AddOffset);
     New->getOperand(3).setIsDead();
 
     // Restore SREG.
     BuildMI(MBB, std::next(II), dl, TII.get(AVR::OUTARr))
-        .addImm(0x3f)
+        .addImm(STI.getIORegSREG())
         .addReg(AVR::R0, RegState::Kill);
 
     // No need to set SREG as dead here otherwise if the next instruction is a
     // cond branch it will be using a dead register.
     BuildMI(MBB, std::next(II), dl, TII.get(SubOpc), AVR::R29R28)
         .addReg(AVR::R29R28, RegState::Kill)
         .addImm(Offset - 63 + 1);
 

llvm/lib/Target/AVR/AVRSubtarget.h

public:
   unsigned getELFArch() const {
     assert(ELFArch != 0 &&
            "every device must have an associate ELF architecture");
     return ELFArch;
   }
 
   /// Get I/O register address.
   int getIORegRAMPZ(void) const { return 0x3b; }
+  int getIORegSREG(void) const { return 0x3f; }
 
 private:
   /// The ELF e_flags architecture.
   unsigned ELFArch;
 
   // Subtarget feature settings
   // See AVR.td for details.
   bool m_hasSRAM;

llvm/test/CodeGen/AVR/atomics/load16.ll

define i16 @atomic_load_cmp_swap16(i16* %foo) {
   %val = cmpxchg i16* %foo, i16 5, i16 10 acq_rel monotonic
   %value_loaded = extractvalue { i16, i1 } %val, 0
   ret i16 %value_loaded
 }
 
 ; CHECK-LABEL: atomic_load_add16
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RR1:r[0-9]+]], [[RD:(X|Y|Z)]]
 ; CHECK-NEXT: ldd [[RR2:r[0-9]+]], [[RD]]+1
-; CHECK-NEXT: add [[RR1]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: adc [[RR2]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: st [[RD]], [[RR1]]
-; CHECK-NEXT: std [[RD]]+1, [[A:r[0-9]+]]
+; CHECK-NEXT: add [[TMP1:r[0-9]+]], [[RR1]]
+; CHECK-NEXT: adc [[TMP2:r[0-9]+]], [[RR2]]
+; CHECK-NEXT: st [[RD]], [[TMP1]]
+; CHECK-NEXT: std [[RD]]+1, [[TMP2]]
 ; CHECK-NEXT: out 63, r0
 define i16 @atomic_load_add16(i16* %foo) {
   %val = atomicrmw add i16* %foo, i16 13 seq_cst
   ret i16 %val
 }
 
 ; CHECK-LABEL: atomic_load_sub16
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RR1:r[0-9]+]], [[RD:(X|Y|Z)]]
 ; CHECK-NEXT: ldd [[RR2:r[0-9]+]], [[RD]]+1
-; CHECK-NEXT: sub [[RR1]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: sbc [[RR2]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: st [[RD]], [[RR1]]
-; CHECK-NEXT: std [[RD]]+1, [[A:r[0-9]+]]
+; CHECK-NEXT: movw
+; CHECK-NEXT: sub [[TMP1:r[0-9]+]], [[IN1:r[0-9]+]]
+; CHECK-NEXT: sbc [[TMP2:r[0-9]+]], [[IN2:r[0-9]+]]
+; CHECK-NEXT: st [[RD]], [[TMP1]]
+; CHECK-NEXT: std [[RD]]+1, [[TMP2]]
 ; CHECK-NEXT: out 63, r0
 define i16 @atomic_load_sub16(i16* %foo) {
   %val = atomicrmw sub i16* %foo, i16 13 seq_cst
   ret i16 %val
 }
 
 ; CHECK-LABEL: atomic_load_and16
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RR1:r[0-9]+]], [[RD:(X|Y|Z)]]
 ; CHECK-NEXT: ldd [[RR2:r[0-9]+]], [[RD]]+1
-; CHECK-NEXT: and [[RR1]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: and [[RR2]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: st [[RD]], [[RR1]]
-; CHECK-NEXT: std [[RD]]+1, [[A:r[0-9]+]]
+; CHECK-NEXT: and [[TMP1:r[0-9]+]], [[RR1]]
+; CHECK-NEXT: and [[TMP2:r[0-9]+]], [[RR2]]
+; CHECK-NEXT: st [[RD]], [[TMP1]]
+; CHECK-NEXT: std [[RD]]+1, [[TMP2]]
 ; CHECK-NEXT: out 63, r0
 define i16 @atomic_load_and16(i16* %foo) {
   %val = atomicrmw and i16* %foo, i16 13 seq_cst
   ret i16 %val
 }
 
 ; CHECK-LABEL: atomic_load_or16
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RR1:r[0-9]+]], [[RD:(X|Y|Z)]]
 ; CHECK-NEXT: ldd [[RR2:r[0-9]+]], [[RD]]+1
-; CHECK-NEXT: or [[RR1]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: or [[RR2]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: st [[RD]], [[RR1]]
-; CHECK-NEXT: std [[RD]]+1, [[A:r[0-9]+]]
+; CHECK-NEXT: or [[TMP1:r[0-9]+]], [[RR1]]
+; CHECK-NEXT: or [[TMP2:r[0-9]+]], [[RR2]]
+; CHECK-NEXT: st [[RD]], [[TMP1]]
+; CHECK-NEXT: std [[RD]]+1, [[TMP2]]
 ; CHECK-NEXT: out 63, r0
 define i16 @atomic_load_or16(i16* %foo) {
   %val = atomicrmw or i16* %foo, i16 13 seq_cst
   ret i16 %val
 }
 
 ; CHECK-LABEL: atomic_load_xor16
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RR1:r[0-9]+]], [[RD:(X|Y|Z)]]
 ; CHECK-NEXT: ldd [[RR2:r[0-9]+]], [[RD]]+1
-; CHECK-NEXT: eor [[RR1]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: eor [[RR2]], [[TMP:r[0-9]+]]
-; CHECK-NEXT: st [[RD]], [[RR1]]
-; CHECK-NEXT: std [[RD]]+1, [[A:r[0-9]+]]
+; CHECK-NEXT: eor [[TMP1:r[0-9]+]], [[RR1]]
+; CHECK-NEXT: eor [[TMP2:r[0-9]+]], [[RR2]]
+; CHECK-NEXT: st [[RD]], [[TMP1]]
+; CHECK-NEXT: std [[RD]]+1, [[TMP2]]
 ; CHECK-NEXT: out 63, r0
 define i16 @atomic_load_xor16(i16* %foo) {
   %val = atomicrmw xor i16* %foo, i16 13 seq_cst
   ret i16 %val
 }
 
 ; CHECK-LABEL: atomic_load_nand16
 ; CHECK: call __sync_fetch_and_nand_2

llvm/test/CodeGen/AVR/atomics/load8.ll

define i8 @atomic_load_cmp_swap8(i8* %foo) {
   %value_loaded = extractvalue { i8, i1 } %val, 0
   ret i8 %value_loaded
 }
 
 ; CHECK-LABEL: atomic_load_add8
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RD:r[0-9]+]], [[RR:(X|Y|Z)]]
-; CHECK-NEXT: add [[RD]], [[RR1:r[0-9]+]]
-; CHECK-NEXT: st [[RR]], [[RD]]
+; CHECK-NEXT: add [[RR1:r[0-9]+]], [[RD]]
+; CHECK-NEXT: st [[RR]], [[RR1]]
 ; CHECK-NEXT: out 63, r0
 define i8 @atomic_load_add8(i8* %foo) {
   %val = atomicrmw add i8* %foo, i8 13 seq_cst
   ret i8 %val
 }
 
 ; CHECK-LABEL: atomic_load_sub8
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RD:r[0-9]+]], [[RR:(X|Y|Z)]]
-; CHECK-NEXT: sub [[RD]], [[RR1:r[0-9]+]]
-; CHECK-NEXT: st [[RR]], [[RD]]
+; CHECK-NEXT: mov [[RR1:r[0-9]+]], [[RD]]
+; CHECK-NEXT: sub [[RR1]], [[RR2:r[0-9]+]]
+; CHECK-NEXT: st [[RR]], [[RR1]]
 ; CHECK-NEXT: out 63, r0
 define i8 @atomic_load_sub8(i8* %foo) {
   %val = atomicrmw sub i8* %foo, i8 13 seq_cst
   ret i8 %val
 }
 
 ; CHECK-LABEL: atomic_load_and8
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RD:r[0-9]+]], [[RR:(X|Y|Z)]]
-; CHECK-NEXT: and [[RD]], [[RR1:r[0-9]+]]
-; CHECK-NEXT: st [[RR]], [[RD]]
+; CHECK-NEXT: and [[RR1:r[0-9]+]], [[RD]]
+; CHECK-NEXT: st [[RR]], [[RR1]]
 ; CHECK-NEXT: out 63, r0
 define i8 @atomic_load_and8(i8* %foo) {
   %val = atomicrmw and i8* %foo, i8 13 seq_cst
   ret i8 %val
 }
 
 ; CHECK-LABEL: atomic_load_or8
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RD:r[0-9]+]], [[RR:(X|Y|Z)]]
-; CHECK-NEXT: or [[RD]], [[RR1:r[0-9]+]]
-; CHECK-NEXT: st [[RR]], [[RD]]
+; CHECK-NEXT: or [[RR1:r[0-9]+]], [[RD]]
+; CHECK-NEXT: st [[RR]], [[RR1]]
 ; CHECK-NEXT: out 63, r0
 define i8 @atomic_load_or8(i8* %foo) {
   %val = atomicrmw or i8* %foo, i8 13 seq_cst
   ret i8 %val
 }
 
 ; CHECK-LABEL: atomic_load_xor8
 ; CHECK:      in r0, 63
 ; CHECK-NEXT: cli
 ; CHECK-NEXT: ld [[RD:r[0-9]+]], [[RR:(X|Y|Z)]]
-; CHECK-NEXT: eor [[RD]], [[RR1:r[0-9]+]]
-; CHECK-NEXT: st [[RR]], [[RD]]
+; CHECK-NEXT: eor [[RR1:r[0-9]+]], [[RD]]
+; CHECK-NEXT: st [[RR]], [[RR1]]
 ; CHECK-NEXT: out 63, r0
 define i8 @atomic_load_xor8(i8* %foo) {
   %val = atomicrmw xor i8* %foo, i8 13 seq_cst
   ret i8 %val
 }
 
 ; CHECK-LABEL: atomic_load_nand8
 ; CHECK: call __sync_fetch_and_nand_1