[AVR] Fix expansion of NEGW

[aykevl]

Summary

The previous expansion used SBCI, which is incorrect because the NEGW pseudo instruction accepts a DREGS operand (2xGPR8) and SBCI only allows LD8 registers. One solution could be to correct the NEGW pseudo instruction, but another solution is to use a different instruction (sbc) that does accept a GPR8 register and therefore allows more freedom to the register allocator.

The output now matches avr-gcc for the following code:

int foo(int n) {
    return -n;
}

I've found this issue using the machine instruction verifier: it was complaining about the wrong register class in NEGWRd.mir.

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

449–450

this line needs to be deleted, otherwise there might be a crash ?

[aykevl]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

449–450

Why?

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

449–450

Ops, I made a mistake. That is not correct.

[benshi001]

Why not commit it? I think this bug fix should be fixed ASAP.

[aykevl]

In D97131#2586533, @benshi001 wrote:

Why not commit it? I think this bug fix should be fixed ASAP.

I prefer doing multiple commits at once and it doesn't affect me other than during unrelated AVR backend development. Would you like to see it fixed right away?

[benshi001]

In D97131#2596186, @aykevl wrote:

In D97131#2586533, @benshi001 wrote:

Why not commit it? I think this bug fix should be fixed ASAP.

I prefer doing multiple commits at once and it doesn't affect me other than during unrelated AVR backend development. Would you like to see it fixed right away?

Yes. I appreciate if you commit your patches. I will do my future work based on it (to avoid potential merge conflict between us)

[benshi001]

In D97131#2596214, @benshi001 wrote:

In D97131#2596186, @aykevl wrote:

In D97131#2586533, @benshi001 wrote:

Why not commit it? I think this bug fix should be fixed ASAP.

I prefer doing multiple commits at once and it doesn't affect me other than during unrelated AVR backend development. Would you like to see it fixed right away?

Yes. I appreciate if you commit your patches. I will do my future work based on it (to avoid potential merge conflict between us)

I have marked 4 of your patches as accepted. I appreciate if you commit them, to avoid potential merge confilcts between us, since I am also working on the same part as you are doing.

[aykevl]

Ok, committed.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

bool AVRExpandPseudo::expand<AVR::NEGWRd>(Block &MBB, BlockIt MBBI) {
   // SREG is always implicitly dead
   MIBHI->getOperand(2).setIsDead();
 
   // Do NEG on the lower byte.
   buildMI(MBB, MBBI, AVR::NEGRd)
       .addReg(DstLoReg, RegState::Define | getDeadRegState(DstIsDead))
       .addReg(DstLoReg, getKillRegState(DstIsKill));
 
-  // Do an extra SBCI.
+  // Do an extra SBC.
   auto MISBCI =
-      buildMI(MBB, MBBI, AVR::SBCIRdK)
+      buildMI(MBB, MBBI, AVR::SBCRdRr)
           .addReg(DstHiReg, RegState::Define | getDeadRegState(DstIsDead))
           .addReg(DstHiReg, getKillRegState(DstIsKill))
-          .addImm(0);
+          .addReg(ZERO_REGISTER);
   if (ImpIsDead)
     MISBCI->getOperand(3).setIsDead();
   // SREG is always implicitly killed
   MISBCI->getOperand(4).setIsKill();
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::CPWRdRr>(Block &MBB, BlockIt MBBI) {
   MachineInstr &MI = *MBBI;

llvm/lib/Target/AVR/AVRInstrInfo.td

def NEGRd : FRd<0b1001,
                   "neg\t$rd",
                   [(set i8:$rd, (ineg i8:$src)), (implicit SREG)]>;
 
   // NEGW Rd+1:Rd
   //
   // Expands to:
   // neg Rd+1
   // neg Rd
-  // sbci Rd+1, 0
+  // sbc Rd+1, r1
   def NEGWRd : Pseudo<(outs DREGS:$rd),
                       (ins DREGS:$src),
                       "negw\t$rd",
                       [(set i16:$rd, (ineg i16:$src)), (implicit SREG)]>;
 }
 
 // TST Rd
 // Test for zero of minus.

llvm/test/CodeGen/AVR/neg.ll

; CHECK-NEXT:    ret
   ret i8 %sub
 }
 
 define i16 @neg16(i16 %x) {
 ; CHECK-LABEL: neg16:
 ; CHECK:       ; %bb.0:
 ; CHECK-NEXT:    neg r25
 ; CHECK-NEXT:    neg r24
-; CHECK-NEXT:    sbci r25, 0
+; CHECK-NEXT:    sbc r25, r1
 ; CHECK-NEXT:    ret
   %sub = sub i16 0, %x
   ret i16 %sub
 }

llvm/test/CodeGen/AVR/pseudo/NEGWRd.mir

 name:            test_negwrd
 body: |
   bb.0.entry:
 
     ; CHECK-LABEL: test_negwrd
 
     ; CHECK:      $r15 = NEGRd $r15, implicit-def dead $sreg
     ; CHECK-NEXT: $r14 = NEGRd $r14
-    ; CHECK-NEXT: $r15 = SBCIRdK $r15, 0, implicit-def $sreg, implicit killed $sreg
+    ; CHECK-NEXT: $r15 = SBCRdRr $r15, $r1, implicit-def $sreg, implicit killed $sreg
 
     $r15r14 = NEGWRd $r15r14, implicit-def $sreg
 ...