[AVR] Fix def state of operands

[aykevl]

Summary

Some instructions (especially mov+pop instructions) were setting the wrong operands. For example, the pop instruction had the register set as a source operand while it is a destination operand (the value is loaded into the register).

I have found these issues using the machine verifier and using manual code inspection.

[aykevl]

• merge D97171

[benshi001]

Generally I am OK with this patch, except there is a lint warning.

[benshi001]

llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp

116

Fix this lint warning.

[aykevl]

Thank you for the review!

llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp

116

I intentionally kept it as-is, to match the existing code. See the golden rule:

If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

bool AVRExpandPseudo::expand<AVR::LDWRdPtr>(Block &MBB, BlockIt MBBI) {
   // Load high byte.
   auto MIBHI = buildMI(MBB, MBBI, OpHi)
     .addReg(CurDstHiReg, RegState::Define)
     .addReg(SrcReg, getKillRegState(SrcIsKill))
     .addImm(1);
 
   if (TmpReg) {
     // Move the high byte into the final destination.
-    buildMI(MBB, MBBI, AVR::MOVRdRr).addReg(DstHiReg).addReg(TmpReg);
+    buildMI(MBB, MBBI, AVR::MOVRdRr, DstHiReg).addReg(TmpReg);
 
     // Move the low byte from the scratch space into the final destination.
-    buildMI(MBB, MBBI, AVR::POPRd).addReg(DstLoReg);
+    buildMI(MBB, MBBI, AVR::POPRd, DstLoReg);
   }
 
   MIBLO.setMemRefs(MI.memoperands());
   MIBHI.setMemRefs(MI.memoperands());
 
   MI.eraseFromParent();
   return true;
 }
bool AVRExpandPseudo::expand<AVR::LDDWRdPtrQ>(Block &MBB, BlockIt MBBI) {
   // Load high byte.
   auto MIBHI = buildMI(MBB, MBBI, OpHi)
     .addReg(CurDstHiReg, RegState::Define)
     .addReg(SrcReg, getKillRegState(SrcIsKill))
     .addImm(Imm + 1);
 
   if (TmpReg) {
     // Move the high byte into the final destination.
-    buildMI(MBB, MBBI, AVR::MOVRdRr).addReg(DstHiReg).addReg(TmpReg);
+    buildMI(MBB, MBBI, AVR::MOVRdRr, DstHiReg).addReg(TmpReg);
 
     // Move the low byte from the scratch space into the final destination.
-    buildMI(MBB, MBBI, AVR::POPRd).addReg(DstLoReg);
+    buildMI(MBB, MBBI, AVR::POPRd, DstLoReg);
   }
 
   MIBLO.setMemRefs(MI.memoperands());
   MIBHI.setMemRefs(MI.memoperands());
 
   MI.eraseFromParent();
   return true;
 }
bool AVRExpandPseudo::expand<AVR::LPMWRdZ>(Block &MBB, BlockIt MBBI) {
 
   // Load high byte.
   auto MIBHI = buildMI(MBB, MBBI, OpHi)
       .addReg(CurDstHiReg, RegState::Define)
       .addReg(SrcReg, getKillRegState(SrcIsKill));
 
   if (TmpReg) {
     // Move the high byte into the final destination.
-    buildMI(MBB, MBBI, AVR::MOVRdRr).addReg(DstHiReg).addReg(TmpReg);
+    buildMI(MBB, MBBI, AVR::MOVRdRr, DstHiReg).addReg(TmpReg);
 
     // Move the low byte from the scratch space into the final destination.
-    buildMI(MBB, MBBI, AVR::POPRd).addReg(DstLoReg);
+    buildMI(MBB, MBBI, AVR::POPRd, DstLoReg);
   }
 
   MIBLO.setMemRefs(MI.memoperands());
   MIBHI.setMemRefs(MI.memoperands());
 
   MI.eraseFromParent();
   return true;
 }
bool AVRExpandPseudo::expand<AVR::ASRW8Rd>(Block &MBB, BlockIt MBBI) {
   TRI->splitReg(DstReg, DstLoReg, DstHiReg);
 
   // Move upper byte to lower byte.
   buildMI(MBB, MBBI, AVR::MOVRdRr)
       .addReg(DstLoReg, RegState::Define | getDeadRegState(DstIsDead))
       .addReg(DstHiReg, getKillRegState(DstIsKill));
 
   // Move the sign bit to the C flag.
-  buildMI(MBB, MBBI, AVR::ADDRdRr).addReg(DstHiReg)
-      .addReg(DstHiReg, RegState::Define | getDeadRegState(DstIsDead))
+  buildMI(MBB, MBBI, AVR::ADDRdRr)
+      .addReg(DstHiReg, RegState::Define, getDeadRegState(DstIsDead))
+      .addReg(DstHiReg, getKillRegState(DstIsKill) | getDeadRegState(DstIsDead))
       .addReg(DstHiReg, getKillRegState(DstIsKill));
 
   // Set upper byte to 0 or -1.
   auto MIBHI =
       buildMI(MBB, MBBI, AVR::SBCRdRr)
           .addReg(DstHiReg, RegState::Define | getDeadRegState(DstIsDead))
           .addReg(DstHiReg, getKillRegState(DstIsKill))
           .addReg(DstHiReg, getKillRegState(DstIsKill));

llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp

if (Imm > 63) {
     // Store the value in the source register to the address
     // pointed to by the pointer register.
     buildMI(MBB, MBBI, AVR::STWPtrRr)
       .addReg(Ptr.getReg())
       .addReg(Src.getReg(), getKillRegState(Src.isKill()));
 
     // Pop the original state of the pointer register.
     buildMI(MBB, MBBI, AVR::POPWRd)
-      .addReg(Ptr.getReg(), getKillRegState(Ptr.isKill()));
+      .addDef(Ptr.getReg(), getKillRegState(Ptr.isKill()));
 
     MI.removeFromParent();
   }
 
   return false;
 }
 
 bool AVRRelaxMem::runOnInstruction(Block &MBB, BlockIt MBBI) {

llvm/test/CodeGen/AVR/relax-mem/STDWPtrQRr.mir

bb.0.entry:
 
     ; We shouldn't expand things which already have 6-bit imms.
     ; CHECK-NEXT: STDWPtrQRr $r29r28, 0, $r1r0
     STDWPtrQRr $r29r28, 0, $r1r0
 
     ; CHECK-NEXT: PUSHWRr $r29r28, implicit-def $sp, implicit $sp
     ; CHECK-NEXT: $r29r28 = SBCIWRdK $r29r28, -64, implicit-def $sreg, implicit $sreg
     ; CHECK-NEXT: STWPtrRr $r29r28, $r1r0
-    ; CHECK-NEXT: POPWRd $r29r28, implicit-def $sp, implicit $sp
+    ; CHECK-NEXT: $r29r28 = POPWRd implicit-def $sp, implicit $sp
     STDWPtrQRr $r29r28, 64, $r1r0
 ...