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

[Ben Shi via Phabricator via llvm-commits]

benshi001 added inline comments.


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


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1897
+    // Continue shifts with the leftover registers.
+    Regs = Regs.drop_front(1);
+
----------------
aykevl wrote:
> benshi001 wrote:
> > benshi001 wrote:
> > > I am also confused by here as your did for the left shift, the `std::pair(ShrExtendReg, 0)` is put at the head of `Regs`, and then dropped by `drop_front(1)` ? Is it necessary to create a temporary copy of `Regs` ?
> > If `Regs.drop_front(1);`, is the above `Regs[0] = std::pair(ShrExtendReg, 0);` redundant ?
> > If `Regs.drop_front(1);`, is the above `Regs[0] = std::pair(ShrExtendReg, 0);` redundant ?
> 
> No. When shifting right by 8, `Regs[0] = std::pair(ShrExtendReg, 0);` directly modifies `Registers[0]`.  After `Regs = Regs.drop_front(1)`, `Regs[0]` refers to `Registers[1]`, `Regs[1]` refers to `Registers[2]`, and `Regs[2]` refers to `Registers[3]`. This means that if you logically shift right by 16, in the first iteration `Regs[0]` will zero `Registers[0]` but in the second iteration `Regs[0]` will zero `Registers[1]`.
> 
> `Regs` is simply a view on `Registers`. It is not a separate array.
> (This is very similar to slices in Go, which is the language I often use).
I see. Your way is good !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140570