[AVR] Optimize the 16-bit NEGW pseudo instruction

[benshi001]

Summary

Optimize "NEGW 'Rd+1 : Rd' to

neg Rd+1
neg Rd
sbci Rd+1, 0

[benshi001]

ping

[aykevl]

I think it would be a good idea to add assembly level tests in llvm/test/CodeGen/AVR/neg.ll (see for example llvm/test/CodeGen/AVR/add.ll).

llvm/lib/Target/AVR/AVRInstrInfo.td

747

Is this correct? I only see one neg while both avr-gcc (https://godbolt.org/z/hETcE9) and the code in AVRExpandPseudoInsts.cpp seem to suggest two neg instructions are needed.

[benshi001]

llvm/lib/Target/AVR/AVRInstrInfo.td

747

This is a previous typo, thanks. Already fixed.

[benshi001]

In D88658#2366703, @aykevl wrote:

I think it would be a good idea to add assembly level tests in llvm/test/CodeGen/AVR/neg.ll (see for example llvm/test/CodeGen/AVR/add.ll).

The asm level tests are added in llvm/test/CodeGen/AVR/neg.ll, thanks.

[aykevl]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

442

I do not think this is true: the C flag (part of SREG) is used in sbci.

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

442

Fixed. Thanks.

[dylanmckay]

Looks good to me, thanks for the patch!

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

bool AVRExpandPseudo::expand<AVR::COMWRd>(Block &MBB, BlockIt MBBI) {
   if (ImpIsDead)
     MIBHI->getOperand(2).setIsDead();
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
+bool AVRExpandPseudo::expand<AVR::NEGWRd>(Block &MBB, BlockIt MBBI) {
+  MachineInstr &MI = *MBBI;
+  Register DstLoReg, DstHiReg;
+  Register DstReg = MI.getOperand(0).getReg();
+  bool DstIsDead = MI.getOperand(0).isDead();
+  bool DstIsKill = MI.getOperand(1).isKill();
+  bool ImpIsDead = MI.getOperand(2).isDead();
+  TRI->splitReg(DstReg, DstLoReg, DstHiReg);
+
+  // Do NEG on the upper byte.
+  auto MIBHI =
+      buildMI(MBB, MBBI, AVR::NEGRd)
+          .addReg(DstHiReg, RegState::Define | getDeadRegState(DstIsDead))
+          .addReg(DstHiReg, getKillRegState(DstIsKill));
+  // SREG is always implicitly dead
+  MIBHI->getOperand(2).setIsDead();
+
+  // Do NEG on the lower byte.
+  auto MIBLO =
+      buildMI(MBB, MBBI, AVR::NEGRd)
+          .addReg(DstLoReg, RegState::Define | getDeadRegState(DstIsDead))
+          .addReg(DstLoReg, getKillRegState(DstIsKill));
+  // SREG is always implicitly dead
+  MIBLO->getOperand(2).setIsDead();
+
+  // Do an extra SBCI.
+  auto MISBCI =
+      buildMI(MBB, MBBI, AVR::SBCIRdK)
+          .addReg(DstHiReg, RegState::Define | getDeadRegState(DstIsDead))
+          .addReg(DstHiReg, getKillRegState(DstIsKill))
+          .addImm(0);
+  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;
   Register SrcLoReg, SrcHiReg, DstLoReg, DstHiReg;
   Register DstReg = MI.getOperand(0).getReg();
   Register SrcReg = MI.getOperand(1).getReg();
   bool DstIsKill = MI.getOperand(0).isKill();
   bool SrcIsKill = MI.getOperand(1).isKill();
   bool ImpIsDead = MI.getOperand(2).isDead();
switch (Opcode) {
     EXPAND(AVR::SBCWRdRr);
     EXPAND(AVR::SBCIWRdK);
     EXPAND(AVR::ANDWRdRr);
     EXPAND(AVR::ANDIWRdK);
     EXPAND(AVR::ORWRdRr);
     EXPAND(AVR::ORIWRdK);
     EXPAND(AVR::EORWRdRr);
     EXPAND(AVR::COMWRd);
+    EXPAND(AVR::NEGWRd);
     EXPAND(AVR::CPWRdRr);
     EXPAND(AVR::CPCWRdRr);
     EXPAND(AVR::LDIWRdK);
     EXPAND(AVR::LDSWRdK);
     EXPAND(AVR::LDWRdPtr);
     EXPAND(AVR::LDWRdPtrPi);
     EXPAND(AVR::LDWRdPtrPd);
   case AVR::LDDWRdYQ: //:FIXME: remove this once PR13375 gets fixed

llvm/lib/Target/AVR/AVRInstrInfo.td

Defs = [SREG] in
   // Expands to:
   // com Rd
   // com Rd+1
   def COMWRd : Pseudo<(outs DREGS:$rd),
                       (ins DREGS:$src),
                       "comw\t$rd",
                       [(set i16:$rd, (not i16:$src)), (implicit SREG)]>;
 
-  //:TODO: optimize NEG for wider types
   def NEGRd : FRd<0b1001,
                   0b0100001,
                   (outs GPR8:$rd),
                   (ins GPR8:$src),
                   "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
+  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.
 // This operation is identical to a `Rd AND Rd`.
 def : InstAlias<"tst\t$rd", (ANDRdRr GPR8:$rd, GPR8:$rd)>;
 
 // SBR Rd, K

llvm/test/CodeGen/AVR/neg.ll

 ; RUN: llc < %s -march=avr | FileCheck %s
 
 define i8 @neg8(i8 %x) {
 ; CHECK-LABEL: neg8:
 ; CHECK: neg r24
   %sub = sub i8 0, %x
   ret i8 %sub
 }
+
+define i16 @neg16(i16 %x) {
+; CHECK-LABEL: neg16:
+; CHECK:       neg r25
+; CHECK-next:  neg r24
+; CHECK-next:  sbci r25, 0
+; CHECK-next:  ret
+  %sub = sub i16 0, %x
+  ret i16 %sub
+}

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

+# RUN: llc -O0 -run-pass=avr-expand-pseudo %s -o - | FileCheck %s
+
+# This test checks the expansion of the 16-bit NEG pseudo instruction.
+
+--- |
+  target triple = "avr--"
+  define void @test_negwrd() {
+  entry:
+    ret void
+  }
+...
+
+---
+name:            test_negwrd
+body: |
+  bb.0.entry:
+
+    ; CHECK-LABEL: test_negwrd
+
+    ; CHECK:      $r15 = NEGRd $r15, implicit-def dead $sreg
+    ; CHECK-NEXT: $r14 = NEGRd $r14, implicit-def dead $sreg
+    ; CHECK-NEXT: $r15 = SBCIRdK $r15, 0, implicit-def $sreg, implicit killed $sreg
+
+    $r15r14 = NEGWRd $r15r14, implicit-def $sreg
+...