[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:1867
+    // Continue shifts with the leftover registers.
+    Regs = Regs.drop_back(1);
+
----------------
benshi001 wrote:
> benshi001 wrote:
> > I am a bit confused about here,
> > 
> > `std::pair(Out, 0)` is first put to the tail, then is dropped by `drop_back(1)` ?
> > 
> > In the first round of this loop, `Regs` comes in with `size==4`, then in the next round, `Regs` only keeps 3 elements before the moves?
> If the last element is dropped, is the above `Regs[Regs.size() - 1] = std::pair(Out, 0);` redundant ?
> `std::pair(Out, 0)` is first put to the tail, then is dropped by `drop_back(1)` ?

Correct. This is a `MutableArrayRef` and is modified in-place (it directly modifies the `Registers` array inside `insertWideShift`). For example, when shifting left by 8 the least significant register in the `Registers` array (`Registers[3]`) is zeroed. The `Regs = Regs.drop_back(1)` line only modifies the _view_ on the underlying array. That means that in the next iteration of the loop, `Regs` will only have a length of 3 (while `Registers` still has a length of four).

See my next comment, where it is hopefully more clear.


================
Comment at: llvm/lib/Target/AVR/AVRISelLowering.cpp:1897
+    // Continue shifts with the leftover registers.
+    Regs = Regs.drop_front(1);
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140570