[PATCH] D56474: [ARM] [NEON] Add ROTR/ROTL lowering

[easyaspi314 (Devin) via Phabricator via llvm-commits]

easyaspi314 planned changes to this revision.
easyaspi314 marked an inline comment as done.
easyaspi314 added a comment.

In D56474#1351603 <https://reviews.llvm.org/D56474#1351603>, @efriedma wrote:

> I'm not sure this is really the best approach... essentially, there are two relevant transforms here:
>
> 1. A rotate by a multiple of 8 can be transformed into a shuffle.  I guess the only case that's really relevant on ARM is vrev, since there aren't any other single-instruction shifts that correspond to a rotate, so maybe it's okay to just special-case here.


Yeah, also, you need to load the pattern.

> 2. `(OR X, (SRL Y, N))` can be transformed to VSRI if X has enough known trailing zeros.  You can special-case rotates (or slightly more generally, FSHL/FSHR), but it's not much harder to handle the general case.

True. I should probably implement that.

> I'm also a little concerned that the VSRI could actually be slower in certain cases... if you look at timings for a Cortex-A57, 128-bit VSRI takes two cycles throughput to execute, as opposed to one for a regular shift.

Well VSRI takes care of both `VSHR` and `VORR`. It doesn't save any time, but it saves space.



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:8038
+    Amount = DAG.getNode(ISD::SUB, DL, VT, DAG.getConstant(Bits, DL, ShiftType), Amount);
+    Value = DAG.getNode(Left ? ISD::SHL : ISD::SRL, DL, VT, Value, Amount);
+    return DAG.getNode(ISD::OR, DL, VT, Value, Temporary);
----------------
```
SDValue Temporary = DAG.getNode(Left ? ISD::SHL : ISD::SRL, DL, VT, Value, Amount);
Value = DAG.getNode(Left ? ISD::SHL : ISD::SRL, DL, VT, Value, Amount);
```

```
Left ? ISD::SHL : ISD::SRL
Left ? ISD::SHL : ISD::SRL
```

Wow. I'm an idiot.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56474