[PATCH] D140570: [AVR] Optimize 32-bit shift: move bytes around

[Ayke via Phabricator via llvm-commits]

aykevl added inline comments.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1849
   MachineRegisterInfo &MRI = BB->getParent()->getRegInfo();
   DebugLoc dl = MI.getDebugLoc();
 
----------------
benshi001 wrote:
> Can this be `const DebugLoc dl` ?
This is part of D140569.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1853
+  // zero the bottom registers.
+  auto MSBReg = Regs[0];
+  Register ShrExtendReg = 0;
----------------
benshi001 wrote:
> These two variables are only used inside `while (ShiftAmt >= 8)`, so how about moving them down to the head of the loop ?
Fixed.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1863
+    Register Out = MRI.createVirtualRegister(&AVR::GPR8RegClass);
+    BuildMI(*BB, MI, dl, TII.get(AVR::COPY), Out).addReg(STI.getZeroRegister());
+    Regs[Regs.size() - 1] = std::pair(Out, 0);
----------------
benshi001 wrote:
> Do we need to do `AVR::COPY` for each iteration ? Can it be a unique `Register Out` shared by all iterations, and even shared with  `lshr` ?
Interesting idea! I've tried it, and it seems to work just as well.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1878
+    // Zero or sign extend the most significant register.
+    if (ShrExtendReg == 0) {
+      ShrExtendReg = MRI.createVirtualRegister(&AVR::GPR8RegClass);
----------------
benshi001 wrote:
> I think this `if (ShrExtendReg == 0)` can be moved out of current loop, something like
> 
> ```
> Register ShrExtendReg = 0;
> if (ShiftAmt >= 8) {
>   auto &MSBReg = Regs[0];
>   ShrExtendReg = MRI.createVirtualRegister(&AVR::GPR8RegClass);
>   if (ArithmeticShift) {
>     ...
>   } else {
>     ...
>   }
> }
> while (ShiftAmt >= 8) {
>   Regs[0] = std::pair(ShrExtendReg, 0);
>   Regs = Regs.drop_front(1);
>   ShiftAmt -= 8;
> }
> ```
> 
> Actually 
> 1. we need not check `if (ShrExtendReg == 0)` time and time again.
> 2. we need not expose `auto &MSBReg = Regs[0]` which is only used locally.
Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140570