[PATCH] D62672: [ARM] Add MVE vector shift instructions.

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham marked 7 inline comments as done.
simon_tatham added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:785
+
+def VSHLC : MVE_p<(outs rGPR:$RdmDest, MQPR:$Qd), (ins MQPR:$QdSrc, rGPR:$RdmSrc,
+                  long_shift:$imm), NoItinerary, "vshlc", "", "$QdSrc, $RdmSrc, $imm",
----------------
ostannard wrote:
> Why are the R and Q registers in the opposite order in the input and output operands?
I think this one was intentional, because it reflects the logical order of the operand pairs. `VSHLC` shifts a Q-register left, filling in the rightmost bits from a GPR, and as output it produces a GPR full of the bits shifted off from the left. So you start with Qx,Ry which logically make up a single bit sequence if arranged in that order, and you end up with Ry,Qx which now make up a shifted version of the same bit sequence if arranged in //that// order.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:815
+
+class t2VMOVL<string iname, string suffix, bits<2> sz, bit U,
+              list<dag> pattern=[]>
----------------
ostannard wrote:
> Some of these names begin with "t2" (matching the other Thumb2 instructions), and some don't. Unless there's a pattern which I've missed, could pick one naming convention and stick to it?
I suspect this was just a case of 'add a prefix wherever it's necessary to prevent a clash with an existing instruction'.

Some MVE instructions have already been committed, so I've opened D63492 to make those more consistent. Also revised D62671 in line with this comment. I'll try to arrange that all the MVE instructions have names beginning `MVE_`.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5984
 
-  if (isMnemonicVPTPredicable(Mnemonic, ExtraToken)) {
+  if (isMnemonicVPTPredicable(Mnemonic, ExtraToken) && Mnemonic != "vmovlt" &&
+      Mnemonic != "vshllt" && Mnemonic != "vrshrnt" && Mnemonic != "vshrnt" &&
----------------
ostannard wrote:
> What are these special cases for? Could this be moved into isMnemonicVPTPredicable?
I'm a bit unclear on this, but I //think// the problem here is that `isMnemonicVPTPredicable` has to be able to answer questions about the version of the mnemonic //with// a T or E suffix (because it's called by `SplitMnemonic`) and //also// about the version without one (because it's called by `getMnemonicAcceptInfo` after the split).

On that basis, these special cases are all things that `isMnemonicVPTPredicable` properly ought to return true for, either because they are things on which a T/E suffix would be legal (e.g. `vqshrnt`, in which the existing t means 'top half' for a narrowing instruction), or because they actually have one (`vmovlt`, unless that's a scalar `vmov` with an ordinary `lt` conditional suffix).

If we broke up `isMnemonicVPTPredicable` into two separate functions, with semantics "is this a mnemonic that might already include a T/E suffix?" and "is this a mnemonic that it would be legal to put a T/E suffix on the end of?", then we could probably get rid of most of these special cases ... (a) provided I haven't completely misunderstood all of this; (b) at the cost of making `isMnemonicVPTPredicable` itself hairier by more than we save here; (c) and it wouldn't even get rid of //all// of them, because I think the reason `vmovlt` is treated unusually here is to handle the lexing ambiguity between `vmovl` `t` and `vmov` `lt`.


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:6089
+
+  // Depending on the (U)nsignedness, (T)op or bottom and size of the element
+  // (size16) we pick different Opcodes.  We do this using an array of opcodes
----------------
ostannard wrote:
> Is this not already handled by the table-generated disassembler?
I think this custom decoder was written because of the combined size + shift count field in the encoding for these instructions. But I've found an alternative way to express it in pure Tablegen, so yes, it looks as if we can get rid of this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62672