[PATCH] D131867: [AVR] Do not emit instructions invalid for attiny10

[Ayke via Phabricator via llvm-commits]

aykevl added a comment.

Please take another look! A lot of things have changed.



================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:1141-1147
+    // Add offset. The offset can be 0 when expanding this instruction from the
+    // more specific STWPtrRr instruction.
+    if (Imm != 0) {
+      buildMI(MBB, MBBI, AVR::SUBIWRdK, DstReg)
+          .addReg(DstReg, RegState::Kill)
+          .addImm(0x10000 - Imm);
     }
----------------
I've changed the behavior here slightly because that's what I did in the previous version of this patch and because it's more efficient. It's better to subtract and then add the pointer than to push, subtract, and pop the previous value.


================
Comment at: llvm/lib/Target/AVR/AVRFrameLowering.cpp:466
+            (Opcode != AVR::STDPtrQRr) && (Opcode != AVR::STDWPtrQRr) &&
+            (Opcode != AVR::FRMIDX)) {
           continue;
----------------
I think this was a preexisting bug that simply never surfaced. It was necessary to fix to get parameter loads from the stack to work correctly.


================
Comment at: llvm/lib/Target/AVR/AVRRegisterInfo.cpp:172-183
+    // Copy the frame pointer.
+    if (STI.hasMOVW()) {
+      BuildMI(MBB, MI, dl, TII.get(AVR::MOVWRdRr), DstReg)
+          .addReg(AVR::R29R28);
+    } else {
+      Register DstLoReg, DstHiReg;
+      splitReg(DstReg, DstLoReg, DstHiReg);
----------------
Instead of changing the instruction, the new code creates a new movw (or two new movs) and removes the FRMIDX instruction. This seems cleaner to me and also fixes a potential bug where the MOVRdRr instruction had an implicit SREG operand (which is weird).


================
Comment at: llvm/lib/Target/AVR/AVRRegisterInfo.cpp:210
+  int MaxOffset = 62;
+  if (STI.hasTinyEncoding()) {
+    MaxOffset = 0;
----------------
benshi001 wrote:
> It seems better to be 
> 
> ```
> int MaxOffset = STI.hasTinyEncoding() ? 0 : 62;
> ```
> 
> And you can change to that when committing your patch.
Fixed.


================
Comment at: llvm/test/CodeGen/AVR/calling-conv/c/tiny.ll:43-44
 ;        while %d(i16) is passed via the stack.
-; FIXME: The `ldd` instruction is invalid on avrtiny, this test just shows
-;        how arguments are passed.
 define i16 @foo3(i16 %a, i16 %b, i16 %c, i16 %d) {
----------------
Fixed the `ldd`!


================
Comment at: llvm/test/CodeGen/AVR/calling-conv/c/tiny.ll:49-59
 ; CHECK-NEXT:    in r29, 62
-; CHECK-NEXT:    ldd r30, Y+5
-; CHECK-NEXT:    ldd r31, Y+6
+; CHECK-NEXT:    in r16, 63
+; CHECK-NEXT:    subi r28, 251
+; CHECK-NEXT:    sbci r29, 255
+; CHECK-NEXT:    ld r30, Y+
+; CHECK-NEXT:    ld r31, Y+
+; CHECK-NEXT:    subi r28, 2
----------------
The code here is really terrible but it should be correct. I hope to optimize this in a later pass with a late pass that merges pointer adjustments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131867/new/

https://reviews.llvm.org/D131867