[AVR] Fix a bug of register allocation

[benshi001]

Summary

Fixes https://github.com/llvm/llvm-project/issues/52839

[benshi001]

llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp

171

the previous variable AM is not longer used, so merge it to the if condition.

378

The deleted operand RegZ triggered the ran out of register error, actually we need not spiecify any implicit operands when calling getMachineNode.

393

We already have ReplaceUses(SDValue(N, 1), SDValue(ResNode, 1)); in the end of this function, any same lines in other places of current function AVRDAGToDAGISel::select<ISD::LOAD>(SDNode *N) are deleted.

llvm/test/CodeGen/AVR/lpmx.ll

2

-O1 / -O2 / -O3 are normal, and only -O0 triggers the error.

[benshi001]

llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp

185

This form is better for future patches which will add ELPM.

[aykevl]

Can you explain what the bug is and why this patch fixes it? I find it hard to interpret the diff.

[benshi001]

In D116399#3218107, @aykevl wrote:

Can you explain what the bug is and why this patch fixes it? I find it hard to interpret the diff.

Sorry, I should make some comments on my changes.

The key reason leaded to the crash was the line in previous code

CurDAG->getMachineNode(LPMOpc, DL, VT, MVT::i16, MVT::Other, Ptr, RegZ);

in which the implicit operand RegZ was explicitly specified in call to getMachineNode, and I remove the RegZ and change the above line to

CurDAG->getMachineNode(LPMOpc, DL, VT, MVT::i16, MVT::Other, Ptr);

then the crash disappears.

Actually there are four getMachineNode get RegZ removed. and others changes are NFC (just simplification for my future patches.)

[benshi001]

In D116399#3218107, @aykevl wrote:

Can you explain what the bug is and why this patch fixes it? I find it hard to interpret the diff.

The bug is recorded at https://github.com/llvm/llvm-project/issues/52839

There is a crash caused by the implicit operand RegZ is explicitly specified when calling CurDAG->getMachineNode.

[benshi001]

llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp

171

All changes in this function selectIndexedProgMemLoad are non functional changes, and they just make the code more clear.

184

otherwise (Offs != 2) Opcode is still 0, which means no match.

llvm/lib/Target/AVR/AVRISelDAGToDAG.cpp

bool AVRDAGToDAGISel::selectIndexedLoad(SDNode *N) {
   ReplaceUses(N, ResNode);
   CurDAG->RemoveDeadNode(N);
 
   return true;
 }
 
 unsigned AVRDAGToDAGISel::selectIndexedProgMemLoad(const LoadSDNode *LD,
                                                    MVT VT) {
-  ISD::MemIndexedMode AM = LD->getAddressingMode();
-
   // Progmem indexed loads only work in POSTINC mode.
-  if (LD->getExtensionType() != ISD::NON_EXTLOAD || AM != ISD::POST_INC) {
+  if (LD->getExtensionType() != ISD::NON_EXTLOAD ||
+      LD->getAddressingMode() != ISD::POST_INC)
     return 0;
-  }
 
   unsigned Opcode = 0;
   int Offs = cast<ConstantSDNode>(LD->getOffset())->getSExtValue();
 
   switch (VT.SimpleTy) {
-  case MVT::i8: {
-    if (Offs != 1) {
-      return 0;
-    }
+  case MVT::i8:
+    if (Offs == 1)
       Opcode = AVR::LPMRdZPi;
     break;
-  }
-  case MVT::i16: {
-    if (Offs != 2) {
-      return 0;
-    }
+  case MVT::i16:
+    if (Offs == 2)
       Opcode = AVR::LPMWRdZPi;
     break;
-  }
   default:
-    return 0;
+    break;
   }
 
   return Opcode;
 }
 
 bool AVRDAGToDAGISel::SelectInlineAsmMemoryOperand(
     const SDValue &Op, unsigned ConstraintCode, std::vector<SDValue> &OutOps) {
   assert((ConstraintCode == InlineAsm::Constraint_m ||
template <> bool AVRDAGToDAGISel::select<ISD::LOAD>(SDNode *N) {
   SDValue Ptr = LD->getBasePtr();
   SDNode *ResNode;
   SDLoc DL(N);
 
   Chain = CurDAG->getCopyToReg(Chain, DL, AVR::R31R30, Ptr, SDValue());
   Ptr = CurDAG->getCopyFromReg(Chain, DL, AVR::R31R30, MVT::i16,
                                Chain.getValue(1));
 
-  SDValue RegZ = CurDAG->getRegister(AVR::R31R30, MVT::i16);
-
   // Check if the opcode can be converted into an indexed load.
   if (unsigned LPMOpc = selectIndexedProgMemLoad(LD, VT)) {
     // It is legal to fold the load into an indexed load.
-    ResNode =
-        CurDAG->getMachineNode(LPMOpc, DL, VT, MVT::i16, MVT::Other, Ptr, RegZ);
-    ReplaceUses(SDValue(N, 1), SDValue(ResNode, 1));
+    ResNode = CurDAG->getMachineNode(LPMOpc, DL, VT, MVT::i16, MVT::Other, Ptr);
   } else {
     // Selecting an indexed load is not legal, fallback to a normal load.
     switch (VT.SimpleTy) {
     case MVT::i8:
-      ResNode = CurDAG->getMachineNode(AVR::LPMRdZ, DL, MVT::i8, MVT::Other,
-                                       Ptr, RegZ);
+      ResNode =
+          CurDAG->getMachineNode(AVR::LPMRdZ, DL, MVT::i8, MVT::Other, Ptr);
       break;
     case MVT::i16:
-      ResNode = CurDAG->getMachineNode(AVR::LPMWRdZ, DL, MVT::i16, MVT::Other,
-                                       Ptr, RegZ);
-      ReplaceUses(SDValue(N, 1), SDValue(ResNode, 1));
+      ResNode =
+          CurDAG->getMachineNode(AVR::LPMWRdZ, DL, MVT::i16, MVT::Other, Ptr);
       break;
     default:
       llvm_unreachable("Unsupported VT!");
     }
   }
 
   // Transfer memory operands.
   CurDAG->setNodeMemRefs(cast<MachineSDNode>(ResNode), {LD->getMemOperand()});
 
   ReplaceUses(SDValue(N, 0), SDValue(ResNode, 0));
   ReplaceUses(SDValue(N, 1), SDValue(ResNode, 1));
   CurDAG->RemoveDeadNode(N);
 
   return true;
 }
 
 template <> bool AVRDAGToDAGISel::select<AVRISD::CALL>(SDNode *N) {
   SDValue InFlag;
   SDValue Chain = N->getOperand(0);

llvm/test/CodeGen/AVR/lpmx.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=avr --mcpu=atmega328 -O0 | FileCheck %s
+
+@arr0 = addrspace(1) constant [4 x i16] [i16 123, i16 234, i16 456, i16 67], align 1
+@arr1 = addrspace(1) constant [4 x i8] c"ABCD", align 1
+
+define i16 @foo0(i16 %a) addrspace(1) {
+; CHECK-LABEL: foo0:
+; CHECK:       ; %bb.0: ; %entry
+; CHECK-NEXT:    push r28
+; CHECK-NEXT:    push r29
+; CHECK-NEXT:    in r28, 61
+; CHECK-NEXT:    in r29, 62
+; CHECK-NEXT:    sbiw r28, 2
+; CHECK-NEXT:    in r0, 63
+; CHECK-NEXT:    cli
+; CHECK-NEXT:    out 62, r29
+; CHECK-NEXT:    out 63, r0
+; CHECK-NEXT:    out 61, r28
+; CHECK-NEXT:    std Y+1, r24
+; CHECK-NEXT:    std Y+2, r25
+; CHECK-NEXT:    ldd r24, Y+1
+; CHECK-NEXT:    ldd r25, Y+2
+; CHECK-NEXT:    lsl r24
+; CHECK-NEXT:    rol r25
+; CHECK-NEXT:    subi r24, -lo8(arr0)
+; CHECK-NEXT:    sbci r25, -hi8(arr0)
+; CHECK-NEXT:    movw r30, r24
+; CHECK-NEXT:    lpm r24, Z+
+; CHECK-NEXT:    lpm r25, Z
+; CHECK-NEXT:    adiw r28, 2
+; CHECK-NEXT:    in r0, 63
+; CHECK-NEXT:    cli
+; CHECK-NEXT:    out 62, r29
+; CHECK-NEXT:    out 63, r0
+; CHECK-NEXT:    out 61, r28
+; CHECK-NEXT:    pop r29
+; CHECK-NEXT:    pop r28
+; CHECK-NEXT:    ret
+entry:
+  %a.addr = alloca i16, align 1
+  store i16 %a, i16* %a.addr, align 1
+  %0 = load i16, i16* %a.addr, align 1
+  %arrayidx = getelementptr inbounds [4 x i16], [4 x i16] addrspace(1)* @arr0, i16 0, i16 %0
+  %1 = load i16, i16 addrspace(1)* %arrayidx, align 1
+  ret i16 %1
+}
+
+define i8 @foo1(i16 %a) addrspace(1) {
+; CHECK-LABEL: foo1:
+; CHECK:       ; %bb.0: ; %entry
+; CHECK-NEXT:    push r28
+; CHECK-NEXT:    push r29
+; CHECK-NEXT:    in r28, 61
+; CHECK-NEXT:    in r29, 62
+; CHECK-NEXT:    sbiw r28, 2
+; CHECK-NEXT:    in r0, 63
+; CHECK-NEXT:    cli
+; CHECK-NEXT:    out 62, r29
+; CHECK-NEXT:    out 63, r0
+; CHECK-NEXT:    out 61, r28
+; CHECK-NEXT:    std Y+1, r24
+; CHECK-NEXT:    std Y+2, r25
+; CHECK-NEXT:    ldd r24, Y+1
+; CHECK-NEXT:    ldd r25, Y+2
+; CHECK-NEXT:    subi r24, -lo8(arr1)
+; CHECK-NEXT:    sbci r25, -hi8(arr1)
+; CHECK-NEXT:    movw r30, r24
+; CHECK-NEXT:    lpm r24, Z
+; CHECK-NEXT:    adiw r28, 2
+; CHECK-NEXT:    in r0, 63
+; CHECK-NEXT:    cli
+; CHECK-NEXT:    out 62, r29
+; CHECK-NEXT:    out 63, r0
+; CHECK-NEXT:    out 61, r28
+; CHECK-NEXT:    pop r29
+; CHECK-NEXT:    pop r28
+; CHECK-NEXT:    ret
+entry:
+  %a.addr = alloca i16, align 1
+  store i16 %a, i16* %a.addr, align 1
+  %0 = load i16, i16* %a.addr, align 1
+  %arrayidx = getelementptr inbounds [4 x i8], [4 x i8] addrspace(1)* @arr1, i16 0, i16 %0
+  %1 = load i8, i8 addrspace(1)* %arrayidx, align 1
+  ret i8 %1
+}