[PATCH] D140571: [AVR] Optimize 32-bit shifts: shift by 4 bits

[Ben Shi via Phabricator via llvm-commits]

benshi001 added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1934
+  //   andi r1, 0xf0
+  //   ; shift r0
+  //   swap r0
----------------
I have two concerns here:

1. It would be better to show a 4-byte shift example, other than a 2-byte one, since you are optimizating the previous one.

2. Your inline comments `; shift r1` and `; shift r0` are incorrect, actually the `r1` is also modified by the last instruction. I think you only give the 
 direct instruction sequence is OK enough. 


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1941
+    Register Prev = 0;
+    for (size_t i = 0; i < Regs.size(); i++) {
+      size_t Idx = ShiftLeft ? i : Regs.size() - i - 1;
----------------
Use `I` than 'i' to be conform with other loops in this function.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1946
+          .addReg(Regs[Idx].first, 0, Regs[Idx].second);
+      if (Prev != 0) {
+        Register R = MRI.createVirtualRegister(&AVR::GPR8RegClass);
----------------
I think it would be better to change to `if (i > 0)`. 


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1957
+          .addImm(ShiftLeft ? 0xf0 : 0x0f);
+      if (Prev != 0) {
+        Register R = MRI.createVirtualRegister(&AVR::GPR8RegClass);
----------------
I think it would be better to change to `if (i > 0)`.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1963
+        if (ShiftLeft) {
+          Regs[Idx - 1] = std::pair(R, 0);
+        } else {
----------------
How about 

```
size_t PrevIdx = ShiftLeft ? Idx - 1 : Idx + 1;
Regs[PrevIdx] = std::pair(R, 0);
```


================
Comment at: llvm/test/CodeGen/AVR/shift32.ll:155
+; CHECK-NEXT:    swap r22
+; CHECK-NEXT:    andi r22, 240
+; CHECK-NEXT:    mov r25, r22
----------------
This looks really great! 


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

https://reviews.llvm.org/D140571