[PATCH] D67158: [ARM] Begin adding IR intrinsics for MVE instructions.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks for splitting this up.



================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:710
+defm MVE_VMINV : MVE_VMINMAXV_ty<
+  "vminv", 0b1, int_arm_mve_minv_s, int_arm_mve_minv_u>;
+defm MVE_VMAXV : MVE_VMINMAXV_ty<
----------------
I feel like we should come up with a style and try and stick with it.

The adds/subs below add in VT to the existing instructions and use it in the new Patterns, mixed in with the old patterns.

These ones add the intrinsics to the multiclass so the top level can include the pattern (but also has patterns outside (below) too.

I think there's value in making this somewhat structured, if we can.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:2794
+  foreach ptype = [mkpred<vtype>.p] in {
+  def : Pat<(vtype (fadd (vtype MQPR:$Qm), (vtype MQPR:$Qn))),
+            (vtype (instr (vtype MQPR:$Qm), (vtype MQPR:$Qn)))>;
----------------
Little bit of indenting, please.


================
Comment at: llvm/test/CodeGen/Thumb2/mve-intrinsics/vaddq.ll:4
+
+define arm_aapcs_vfpcc <4 x i32> @test_vaddq_u32(<4 x i32> %a, <4 x i32> %b) {
+; CHECK-LABEL: test_vaddq_u32:
----------------
We probably don't _need_ tests for simple instructions like this, they should be covered elsewhere (fine to leave them if you wish).



================
Comment at: llvm/test/CodeGen/Thumb2/mve-intrinsics/vaddq.ll:24
+
+define arm_aapcs_vfpcc <16 x i8> @test_vaddq_m_s8(<16 x i8> %inactive, <16 x i8> %a, <16 x i8> %b, i16 zeroext %p) {
+; CHECK-LABEL: test_vaddq_m_s8:
----------------
For the rest of the tests, at least for codegen we have tried to fill in all the combinations for type and operations (at least the legal types). It can be useful for making sure nothing is missed (here or in the future when some refactoring happens).

Whether you want to do the same thing here is up to you, or whether you think that having interesting combinations is enough (adds with v16i8, subs with v4f32 for example).


================
Comment at: llvm/test/CodeGen/Thumb2/mve-intrinsics/vaddq.ll:52
+  %1 = tail call <4 x i1> @llvm.arm.mve.pred.i2v.v4i1(i32 %0)
+  %2 = tail call <4 x float> @llvm.arm.mve.sub.predicated.v4f32.v4i1(<4 x float> %a, <4 x float> %b, <4 x i1> %1, <4 x float> %inactive)
+  ret <4 x float> %2
----------------
Do we care about what happens when there is a fp intrinsic but we don't have mve.fp? I presume this will be a fail to select or some sort of legalisation error, which is probably fine considering what is happening.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67158