[PATCH] D123528: [AVR] Always expand STDSPQRr & STDWSPQRr

[Ben Shi via Phabricator via llvm-commits]

benshi001 added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:1216
+
+  assert(MI.getOperand(0).getReg() == AVR::SP && "Unexpected register");
+
----------------
"SP is expected as base pointer"


================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:1220
+  MI.getOperand(0).setReg(STI.getFrameLowering()->hasReservedCallFrame(MF)
+                              ? AVR::R29R28
+                              : AVR::R31R30);
----------------
benshi001 wrote:
> Could you please add cases to show the use of `R29R28` or `R31R30`  in different situations ?
Sorry, ignore this comment.


================
Comment at: llvm/lib/Target/AVR/AVRFrameLowering.cpp:369
-      // instructions.
-      fixStackStores(MBB, MI, TII, AVR::R31R30);
-    } else {
----------------
benshi001 wrote:
> I do not think the removal of `fixStackStores` at here is good, since we seperate the preparation of `R31R30` and the substitution of `STDWSPQRr`-> `STDWPtrQRr`. It makes the code hard to understand, and I also concern there might be other issues introduced.
> 
> So I suggest we keep this `fixStackStores` call.
I suggest a moderate way:

Keep the second `fixStackStores` (with R31R30) and remove the first one (R29R28). And in the `expand<AVR::STDWSPQRr>`, add an extra line 

```
assert(STI.getFrameLowering()->hasReservedCallFrame(MF) &&
       "unexpected STDWSPQRr pseudo instruction");
```


================
Comment at: llvm/test/CodeGen/AVR/stdwstk.ll:2
+; RUN: llc < %s -march=avr -mcpu=atmega328 -O1 | FileCheck %s
+; CHECK-NOT: stdwstk
+
----------------
benshi001 wrote:
> I perfer the `.mir` tests in your previous https://reviews.llvm.org/D114611.
> 
> It would be better to seperate to STDWSPQRr.mir and STDSPQRr.mir.
Sorry, igmore this comment.


================
Comment at: llvm/test/CodeGen/AVR/stdwstk.ll:7-9
+; See:
+; - https://reviews.llvm.org/D114611
+; - https://reviews.llvm.org/D95664
----------------
Remove these lines

```
; See:
; - https://reviews.llvm.org/D114611
; - https://reviews.llvm.org/D95664
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123528