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

[Ayke via Phabricator via llvm-commits]

aykevl added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1934
+  //   andi r1, 0xf0
+  //   ; shift r0
+  //   swap r0
----------------
benshi001 wrote:
> 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. 
Yes, this isn't very clear. I have updated the explanation in a way that better explains what the code does.


================
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);
----------------
benshi001 wrote:
> I think it would be better to change to `if (i > 0)`. 
Hmm, somehow I missed this while rebasing. Fixed.


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


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1963
+        if (ShiftLeft) {
+          Regs[Idx - 1] = std::pair(R, 0);
+        } else {
----------------
benshi001 wrote:
> How about 
> 
> ```
> size_t PrevIdx = ShiftLeft ? Idx - 1 : Idx + 1;
> Regs[PrevIdx] = std::pair(R, 0);
> ```
Not sure which one is clearer but seems good to me. Change applied.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140571