[PATCH] D95586: [ARM] permit PC as destination of LSL

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D95586#2527659 <https://reviews.llvm.org/D95586#2527659>, @rengolin wrote:

> ARS, LSR, ROR have the following warning on the ARM ARM:
>
>> if d == 15 || n == 15 || m == 15 then UNPREDICTABLE;

Then why does llvm/test/MC/Disassembler/ARM/unpredictable-LSL-regform.txt test `LSL`, and not `ARS`, `LSR`, or `ROR`?  (There are other tests llvm/test/MC/Disassembler/ARM/unpredictable-, but there don't seem to be tests for those).

> I'm not sure GAS should be doing this at all... Looks like an oversight.
>
> MOVs and RRX don't in the ARM version, but do in the Thumb2 ones.
>
> However, not all encodings are allowed.
>
> For example, MOVS PC is SUBS, but MOV PC is not allowed. OTOH, RRX only allows the PC is S isn't specified.
>
> Maybe the kernel is using those instructions in the precise way the ARM ARM allows, but this patch is also allowing unpredictable encodings, which we really shouldn't.

Ah, looks like out of `movs, lsl, lsr, asr, ror, and rrx`, the kernel is only using `movs` with `pc` destination, in ARM mode (non-Thumb).  I can restrict the patch to `movs`, though the test case in llvm/test/MC/Disassembler/ARM/unpredictable-LSL-regform.txt would still fail.  And it still begs the question, why is it ok for the immediate operand versions of these to permit `pc` destination?

> but this patch is also allowing unpredictable encodings, which we really shouldn't.

I guess I don't really follow whether the recommendation is to do something else in LLVM, or remove the kernel code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95586