[PATCH] D72934: [ARM,MVE] Support immediate vbicq,vorrq,vmvnq intrinsics.

Mon Jan 20 05:49:28 PST 2020

simon_tatham marked 2 inline comments as done.
simon_tatham added a comment.

In D72934#1829331 <https://reviews.llvm.org/D72934#1829331>, @dmgreen wrote:

> What is the reason that this can't be lowered in tablegen, in the same way as the VMOVimm's are?

In NEON, immediate VBIC is represented as a single MC instruction, which takes its immediate operand already encoded into the NEON format (8 data bits, op and cmode). That's the same format that `ARMISD::VBICIMM` has encoded the operand in after lowering. So you only need one tablegen pattern, which passes the immediate through unchanged between the input and output SDNode types.

In MVE, immediate VBIC is represented as four separate MC instructions, for an 8-bit immediate shifted left by 0, 8, 16 or 24 bits. Each one takes the immediate operand in the 'natural' form, i.e. the numerical value that would be combined into the vector lane and shown in assembly. For example, `MVE_VBICIZ16v4i32` takes an operand such as `0xab0000` which NEON VBIC would represent as `0xab | (control bits << 8)`. So the C++ isel code I've written has to undo the NEON encoding and turn it back into the 'natural' immediate value plus a choice of which MVE opcode to use.

I suppose an alternative would be to rework the MC representation of MVE VBIC/VORR so that they look more like the NEON versions. I don't exactly know why MVE was done differently in the first place (the commit here has my name on it, but it was a team effort). One possibility is that the pseudo-instruction reversed forms `vand` and `vorn` might be hard to represent that way, but I don't know.

> Do you have any tests for what would be invalid bic values under MVE?

True, I suppose I could provide some immediates that are valid for other `VMOVModImmType`s, like `0xabff`, and make sure nothing goes wrong.

================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:12181
       BVN->isConstantSplat(SplatBits, SplatUndef, SplatBitSize, HasAnyUndefs)) {
     if (SplatBitSize <= 64) {
       EVT VbicVT;
----------------
dmgreen wrote:
> This is OK because we are passing OtherModImm to isVMOVModifiedImm, and MVE supports the same patterns as NEON?
Yes: `OtherModImm` only matches values of the form '8-bit number shifted left by a multiple of 8 bits', which is just what MVE VBIC and VORR take as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72934