[AVR] Fix expanding MOVW for overlapping registers

[Patryk27]

Summary

Sometimes the codegen emits a COPY with overlapping registers, such as
this one:

$r25r24 = COPY $r24r23

Our current expansion of such COPY would be wrong, since it'd start
with the lower register first, with the second mov assuming the
registers are not modified in-between:

mov r24, r23
mov r25, r24

(i.e. that's effectively mov r25, r23, which is _ayy ayy bad_.)

This patch improves the expansion by making it detect whether the
registers are overlapping and if so, expanding from the high register
first:

mov r25, r24
mov r24, r23

Because our registers are always paired in descending order (e.g.
there's r25r24, but no r24r25), I think it'd be safe to go with the
high-to-low expansion always (not only if the registers overlap), but
that makes the output a bit more difficult to follow and would require
adjusting the existing tests -- so I went with the more safer & simpler
route.

In the wild, this was found here:
https://github.com/rust-lang/rust/issues/98167.

[benshi001]

llvm/lib/Target/AVR/AVRInstrInfo.cpp

61

Using name CopyHasOverlappingRegs maybe confusing, for the case DestHi == SrcLow. Though there is overlapping for r25r24 -> r24r23, we have to still copy lower byte first.

So I suggest

if (DestLo == SrcHi) {
  ...
} else {
  ...
}

And the long comment is unnecessary, since the logic is clear enough to understand.

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

31

Are these two implicit killed in the two RCALLk necessary ?

[Patryk27]

Apply code review changes

[Patryk27]

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

31

Without any instruction that actually uses the result of that COPY, LLVM seems to generate no code whatsoever - I've thought about RCALLk as a simple way of telling LLVM "trust me, that register is read later".

Those don't have to be killed though, so to simplify the test, I've just changed them to RCALLk @foo, implicit $r24r23.

llvm/lib/Target/AVR/AVRInstrInfo.cpp

 void AVRInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
                                MachineBasicBlock::iterator MI,
                                const DebugLoc &DL, MCRegister DestReg,
                                MCRegister SrcReg, bool KillSrc) const {
   const AVRSubtarget &STI = MBB.getParent()->getSubtarget<AVRSubtarget>();
   const AVRRegisterInfo &TRI = *STI.getRegisterInfo();
   unsigned Opc;
 
-  // Not all AVR devices support the 16-bit `MOVW` instruction.
   if (AVR::DREGSRegClass.contains(DestReg, SrcReg)) {
+    // If our AVR has `movw`, let's emit that; otherwise let's emit two separate
+    // `mov`s.
     if (STI.hasMOVW() && AVR::DREGSMOVWRegClass.contains(DestReg, SrcReg)) {
       BuildMI(MBB, MI, DL, get(AVR::MOVWRdRr), DestReg)
           .addReg(SrcReg, getKillRegState(KillSrc));
     } else {
       Register DestLo, DestHi, SrcLo, SrcHi;
 
       TRI.splitReg(DestReg, DestLo, DestHi);
       TRI.splitReg(SrcReg, SrcLo, SrcHi);
 
-      // Copy each individual register with the `MOV` instruction.
+      bool CopyHasOverlappingRegs = DestLo == SrcHi;
+
+      // Usually, for readability & consistency, we emit:
+      //
+      // ```
+      // mov $DestLo, $SrcLo
+      // mov $DestHi, $SrcHi
+      // ```
+      //
+      // ... but if our registers overlap, then we have to switch the order:
+      //
+      // ```
+      // mov $DestHi, $SrcHi
+      // mov $DestLo, $SrcLo
+      // ```
+      //
+      // ... as not to garble the operation.
+      //
+      // E.g. imagine trying to expand `$r25r24 = COPY $r24r23` - doing it the
+      // first way would yield:
+      //
+      // ```
+      // mov r24, r23
+      // mov r25, r24
+      // ```
+      //
+      // ... which would always set both registers to `r23`.
+      if (CopyHasOverlappingRegs) {
+        BuildMI(MBB, MI, DL, get(AVR::MOVRdRr), DestHi)
+            .addReg(SrcHi, getKillRegState(KillSrc));
+        BuildMI(MBB, MI, DL, get(AVR::MOVRdRr), DestLo)
+            .addReg(SrcLo, getKillRegState(KillSrc));
+      } else {
         BuildMI(MBB, MI, DL, get(AVR::MOVRdRr), DestLo)
             .addReg(SrcLo, getKillRegState(KillSrc));
         BuildMI(MBB, MI, DL, get(AVR::MOVRdRr), DestHi)
             .addReg(SrcHi, getKillRegState(KillSrc));
       }
+    }
   } else {
     if (AVR::GPR8RegClass.contains(DestReg, SrcReg)) {
       Opc = AVR::MOVRdRr;
     } else if (SrcReg == AVR::SP && AVR::DREGSRegClass.contains(DestReg)) {
       Opc = AVR::SPREAD;
     } else if (DestReg == AVR::SP && AVR::DREGSRegClass.contains(SrcReg)) {
       Opc = AVR::SPWRITE;
     } else {

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

+# RUN: llc -O0 %s -o - | FileCheck %s
+
+--- |
+  target triple = "avr--"
+
+  define void @test_copy_nonoverlapping() {
+  entry:
+    ret void
+  }
+
+  define void @test_copy_overlapping() {
+  entry:
+    ret void
+  }
+
+  declare void @foo(i16 %0)
+...
+
+---
+name: test_copy_nonoverlapping
+tracksRegLiveness: true
+body: |
+  bb.0.entry:
+    liveins: $r25r24
+
+    ; CHECK-LABEL: test_copy_nonoverlapping:
+    ; CHECK: mov r22, r24
+    ; CHECK-NEXT: mov r23, r25
+
+    $r23r22 = COPY $r25r24
+    RCALLk @foo, implicit $sp, implicit killed $r24r23
+...
+
+---
+name: test_copy_overlapping
+tracksRegLiveness: true
+body: |
+  bb.0.entry:
+    liveins: $r24r23
+
+    ; CHECK-LABEL: test_copy_overlapping:
+    ; CHECK: mov r25, r24
+    ; CHECK-NEXT: mov r24, r23
+
+    $r25r24 = COPY $r24r23
+    RCALLk @foo, implicit $sp, implicit killed $r25r24
+...

llvm/test/CodeGen/AVR/rust-bug-98167.ll

+; RUN: llc < %s -march=avr | FileCheck %s
+
+; The bug can be found here:
+; https://github.com/rust-lang/rust/issues/98167
+;
+; In this test, `extractvalue` + `call` generate a copy with overlapping
+; registers (`$r25r24 = COPY $r24r23`) that used to be expanded incorrectly.
+
+define void @main() {
+; CHECK-LABEL: main:
+; CHECK: rcall foo
+; CHECK-NEXT: mov r25, r24
+; CHECK-NEXT: mov r24, r23
+; CHECK-NEXT: rcall bar
+  %1 = call { i8, i16 } @foo()
+  %2 = extractvalue { i8, i16 } %1, 1
+  call void @bar(i16 %2)
+  ret void
+}
+
+declare { i8, i16 } @foo()
+declare void @bar(i16 %0)