[ARM] Select MVE add and sub

[dmgreen]

Summary

Simple patterns for MVEC integer vector add and sub instructions.

Initial code by David Sherwood

[samparker]

Bit confusing this, which other patch(es) is this dependent upon?

[dmgreen]

Hello. This is probably dependent upon at least some of the uncommitted MVE instructions being added (I haven't checked if add/sub are in tree yet).

If by confusing you are referring to the changes in lib/Target/ARM/ARMISelLowering.cpp, then yes I can see that. Essentially I am breaking things so that we can work from the position that most targets are usually in - the default set of instructions are legal and we choose specific ones to mark as legal or not legal. As opposed to what the code was previously doing - marking all instructions as expand. No one will be using the MVE instructions yet (and the compiler shouldn't generate them until we enable that), so it's probably fine to not have this as 100% working at the moment.

Our downstream version looks something like this, as an illustration:

for (auto VT : iTypes) {
  addRegisterClass(VT, &ARM::QPRRegClass);
  setOperationAction(ISD::VECTOR_SHUFFLE, VT, Custom);
  setOperationAction(ISD::INSERT_VECTOR_ELT, VT, Custom);
  setOperationAction(ISD::EXTRACT_VECTOR_ELT, VT, Custom);
  setOperationAction(ISD::BUILD_VECTOR, VT, Custom);
  setOperationAction(ISD::MLOAD, VT, Legal);
  setOperationAction(ISD::MSTORE, VT, Legal);
  setOperationAction(ISD::MGATHER, VT, Legal);
  setOperationAction(ISD::SETCC, VT, Custom);
  setOperationAction(ISD::SHL, VT, Custom);
  setOperationAction(ISD::SRA, VT, Custom);
  setOperationAction(ISD::SRL, VT, Custom);
  setOperationAction(ISD::SIGN_EXTEND_INREG, VT, Legal);
  setOperationAction(ISD::ABS, VT, Legal);

  // No native support for these.
  setOperationAction(ISD::UDIV, VT, Expand);
  setOperationAction(ISD::SDIV, VT, Expand);
  setOperationAction(ISD::UREM, VT, Expand);
  setOperationAction(ISD::SREM, VT, Expand);

  // Pre and Post inc are supported
  for (unsigned im = (unsigned)ISD::PRE_INC;
       im != (unsigned)ISD::LAST_INDEXED_MODE; ++im) {
    setIndexedLoadAction(im, VT, Legal);
    setIndexedStoreAction(im, VT, Legal);
  }
}

I can change it to only mark the current supported set of nodes as legal if you things that's better. Eventually we will probably want to remove the for (every opcode) setOperationAction(Opc, VT, Expand); loops.

[dmgreen]

Also, forgot to mention. I was putting this up early to see how people thought about the general layout, before getting a lot of other patches together. The main point being that it puts all the patterns together at the end of the MVE.td file, as opposed to interleaving them with instructions or in the instruction definition patterns.

Simon already mentioned that we could put the patterns into the instructions if we can sort out things like default class parameters. That would obviously involve a bit of a rewrite, but if people think it's a better way to structure the patterns, I'm all ears.

[simon_tatham]

In D63255#1548264, @dmgreen wrote:

Simon already mentioned that we could put the patterns into the instructions if we can sort out things like default class parameters.

I still haven't managed to form a definite opinion on whether that's better, though!

In favour of putting ISel patterns inside the instruction records they go with:

• readability: all the information about an instruction is in the same place and easy to find
• DRYness: when the instruction applies to half a dozen different types with suffixes like .f16, .s32 and the like, the pattern is automatically replicated across the same set of types, without having to write a separate repetition elsewhere (foreach or multiclass or whatever) and make sure to keep the two in sync.

The unusual problem with MVE is the predication. Almost all the vector instructions will ultimately want at least one ISel pattern corresponding to one of the ACLE intrinsics (or rather, to an LLVM intrinsic which the ACLE one will have been converted into by a combination of clang and a header file) which allows the predicated version of the instruction to be used by C code. But some instructions also want patterns corresponding to existing DAG node types, such as ordinary add on vector types – and you can't put both patterns inside the instruction record, because the Pattern field only has room for one. (It's a list<dag>, but the elements of the list are parts of the same pattern, not independent alternatives.)

So is it better to put the intrinsic patterns in the instructions? Or the patterns involving standard DAG nodes (if any)? Or neither, on the basis that if you can't have all the patterns in the instructions then you should instead keep them near each other? No idea, really.

(Also, there's that problem where operands whose default values are specified by virtue of an OperandWithDefaultOps class can't be overridden by ISel patterns. So the intrinsics for the predicated forms of the instructions can't currently be written as ISel patterns at all. But I think that's fixable: I have a draft patch to TableGen already that should remove that constraint, and I'll upload it once I find time to test it a bit more thoroughly.)

[dmgreen]

Rebase for new instruction names and moved the patterns to be near (but not in) the relevant instruction encodings. I think I like this way fine.

[samparker]

llvm/test/CodeGen/Thumb2/mve-simple-arith.ll

2

Is there a reason why we use mve.fp or integer tests?

[dmgreen]

The same test includes fadd and fsub later on. Changes incoming for these patches though, which tests both..

[samparker]

Ok. LGTM

[dmgreen]

Simplified now that this sits on top of D63567. Includes +mve.fp and +mve,+fullfp16 tests.

llvm/lib/Target/ARM/ARMISelLowering.cpp

 }
 
 void ARMTargetLowering::addQRTypeForNEON(MVT VT) {
   addRegisterClass(VT, &ARM::DPairRegClass);
   addTypeForNEON(VT, MVT::v2f64, MVT::v4i32);
 }
 
 void ARMTargetLowering::addMVEITypes() {
-  const MVT iTypes[] = { MVT::v16i8, MVT::v8i16, MVT::v4i32, MVT::v2i64 };
+  const MVT iTypes[] = { MVT::v16i8, MVT::v8i16, MVT::v4i32 };
 
   for (auto VT : iTypes) {
     addRegisterClass(VT, &ARM::QPRRegClass);
-    for (unsigned Opc = 0; Opc < ISD::BUILTIN_OP_END; ++Opc)
-      setOperationAction(Opc, VT, Expand);
-    setOperationAction(ISD::BITCAST, VT, Legal);
-    setOperationAction(ISD::LOAD, VT, Legal);
-    setOperationAction(ISD::STORE, VT, Legal);
   }
+
+  addRegisterClass(MVT::v2i64, &ARM::QPRRegClass);
+  for (unsigned Opc = 0; Opc < ISD::BUILTIN_OP_END; ++Opc)
+    setOperationAction(Opc, MVT::v2i64, Expand);
+  setOperationAction(ISD::BITCAST, MVT::v2i64, Legal);
+  setOperationAction(ISD::LOAD, MVT::v2i64, Legal);
+  setOperationAction(ISD::STORE, MVT::v2i64, Legal);
 }
 
 void ARMTargetLowering::addMVEFPTypes() {
-  const MVT fTypes[] = { MVT::v2f64, MVT::v4f32, MVT::v8f16 };
+  const MVT fTypes[] = { MVT::v4f32, MVT::v8f16 };
 
   for (MVT VT : fTypes) {
     addRegisterClass(VT, &ARM::QPRRegClass);
-    for (unsigned Opc = 0; Opc < ISD::BUILTIN_OP_END; ++Opc)
-      setOperationAction(Opc, VT, Expand);
-    setOperationAction(ISD::BITCAST, VT, Legal);
-    setOperationAction(ISD::LOAD, VT, Legal);
-    setOperationAction(ISD::STORE, VT, Legal);
   }
+
+  addRegisterClass(MVT::v2f64, &ARM::QPRRegClass);
+  for (unsigned Opc = 0; Opc < ISD::BUILTIN_OP_END; ++Opc)
+    setOperationAction(Opc, MVT::v2f64, Expand);
+  setOperationAction(ISD::BITCAST, MVT::v2f64, Legal);
+  setOperationAction(ISD::LOAD, MVT::v2f64, Legal);
+  setOperationAction(ISD::STORE, MVT::v2f64, Legal);
 }
 
 ARMTargetLowering::ARMTargetLowering(const TargetMachine &TM,
                                      const ARMSubtarget &STI)
     : TargetLowering(TM), Subtarget(&STI) {
   RegInfo = Subtarget->getRegisterInfo();
   Itins = Subtarget->getInstrItineraryData();
 

llvm/lib/Target/ARM/ARMInstrMVE.td

class MVE_VADD<string suffix, bits<2> size, list<dag> pattern=[]>
   : MVE_VADDSUB<"vadd", suffix, size, 0b0, pattern>;
 class MVE_VSUB<string suffix, bits<2> size, list<dag> pattern=[]>
   : MVE_VADDSUB<"vsub", suffix, size, 0b1, pattern>;
 
 def MVE_VADDi8  : MVE_VADD<"i8",  0b00>;
 def MVE_VADDi16 : MVE_VADD<"i16", 0b01>;
 def MVE_VADDi32 : MVE_VADD<"i32", 0b10>;
 
+let Predicates = [HasMVEInt] in {
+  def : Pat<(v16i8 (add (v16i8 MQPR:$val1), (v16i8 MQPR:$val2))),
+            (v16i8 (MVE_VADDi8 (v16i8 MQPR:$val1), (v16i8 MQPR:$val2)))>;
+  def : Pat<(v8i16 (add (v8i16 MQPR:$val1), (v8i16 MQPR:$val2))),
+            (v8i16 (MVE_VADDi16 (v8i16 MQPR:$val1), (v8i16 MQPR:$val2)))>;
+  def : Pat<(v4i32 (add (v4i32 MQPR:$val1), (v4i32 MQPR:$val2))),
+            (v4i32 (MVE_VADDi32 (v4i32 MQPR:$val1), (v4i32 MQPR:$val2)))>;
+}
+
 def MVE_VSUBi8  : MVE_VSUB<"i8",  0b00>;
 def MVE_VSUBi16 : MVE_VSUB<"i16", 0b01>;
 def MVE_VSUBi32 : MVE_VSUB<"i32", 0b10>;
 
+let Predicates = [HasMVEInt] in {
+  def : Pat<(v16i8 (sub (v16i8 MQPR:$val1), (v16i8 MQPR:$val2))),
+            (v16i8 (MVE_VSUBi8 (v16i8 MQPR:$val1), (v16i8 MQPR:$val2)))>;
+  def : Pat<(v8i16 (sub (v8i16 MQPR:$val1), (v8i16 MQPR:$val2))),
+            (v8i16 (MVE_VSUBi16 (v8i16 MQPR:$val1), (v8i16 MQPR:$val2)))>;
+  def : Pat<(v4i32 (sub (v4i32 MQPR:$val1), (v4i32 MQPR:$val2))),
+            (v4i32 (MVE_VSUBi32 (v4i32 MQPR:$val1), (v4i32 MQPR:$val2)))>;
+}
+
 class MVE_VQADDSUB<string iname, string suffix, bit U, bit subtract,
                    bits<2> size, list<dag> pattern=[]>
   : MVE_int<iname, suffix, size, pattern> {
 
   let Inst{28} = U;
   let Inst{25-23} = 0b110;
   let Inst{16} = 0b0;
   let Inst{12-10} = 0b000;

llvm/test/CodeGen/Thumb2/mve-simple-arith.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=thumbv8.1m.main-arm-none-eabi -mattr=+mve.fp -verify-machineinstrs %s -o - | FileCheck %s
+
+define arm_aapcs_vfpcc <16 x i8> @add_int8_t(<16 x i8> %src1, <16 x i8> %src2) {
+; CHECK-LABEL: add_int8_t:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    vadd.i8 q0, q0, q1
+; CHECK-NEXT:    bx lr
+entry:
+  %0 = add <16 x i8> %src1, %src2
+  ret <16 x i8> %0
+}
+
+define arm_aapcs_vfpcc <8 x i16> @add_int16_t(<8 x i16> %src1, <8 x i16> %src2) {
+; CHECK-LABEL: add_int16_t:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    vadd.i16 q0, q0, q1
+; CHECK-NEXT:    bx lr
+entry:
+  %0 = add <8 x i16> %src1, %src2
+  ret <8 x i16> %0
+}
+
+define arm_aapcs_vfpcc <4 x i32> @add_int32_t(<4 x i32> %src1, <4 x i32> %src2) {
+; CHECK-LABEL: add_int32_t:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    vadd.i32 q0, q0, q1
+; CHECK-NEXT:    bx lr
+entry:
+  %0 = add nsw <4 x i32> %src1, %src2
+  ret <4 x i32> %0
+}
+
+
+define arm_aapcs_vfpcc <16 x i8> @sub_int8_t(<16 x i8> %src1, <16 x i8> %src2) {
+; CHECK-LABEL: sub_int8_t:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    vsub.i8 q0, q1, q0
+; CHECK-NEXT:    bx lr
+entry:
+  %0 = sub <16 x i8> %src2, %src1
+  ret <16 x i8> %0
+}
+
+define arm_aapcs_vfpcc <8 x i16> @sub_int16_t(<8 x i16> %src1, <8 x i16> %src2) {
+; CHECK-LABEL: sub_int16_t:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    vsub.i16 q0, q1, q0
+; CHECK-NEXT:    bx lr
+entry:
+  %0 = sub <8 x i16> %src2, %src1
+  ret <8 x i16> %0
+}
+
+define arm_aapcs_vfpcc <4 x i32> @sub_int32_t(<4 x i32> %src1, <4 x i32> %src2) {
+; CHECK-LABEL: sub_int32_t:
+; CHECK:       @ %bb.0: @ %entry
+; CHECK-NEXT:    vsub.i32 q0, q1, q0
+; CHECK-NEXT:    bx lr
+entry:
+  %0 = sub nsw <4 x i32> %src2, %src1
+  ret <4 x i32> %0
+}
+