[PATCH] D70181: [MVE] [ARM] Select VQABS

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1955
 
+let Predicates = [HasMVEInt] in {
+  def : Pat<(v16i8 (vselect
----------------
SjoerdMeijer wrote:
> Should this be `HasMVEFloat`?
No – VQABS is an integer instruction. The point is that it takes the absolute value of a signed integer, and gives you back something that fits in the same signed integer type, which means it has to map the largest negative value (say -128) to //one less// than its true absolute value (e.g. +127) or else it still ends up negative.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1961
+                                (v16i1 (ARMvcmp (v16i8 MQPR:$reg),
+                                                (v16i8 (ARMvmovImm (i32 3712))),
+                                                (i32 0))),
----------------
These magic numbers like `3712` and `2688` and so on could do with a comment explaining what they represent. From context it looks as if they're some kind of non-literal immediate encoding used by special node types like `ARMvcmpz`, `ARMvmovImm` and so on – but what real numbers do they represent?


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1963
+                                                (i32 0))),
+                                (v16i8 (ARMvmovImm (i32 3711))),
+                                (sub
----------------
This integer 3711 is different from the 3712 two lines above it. But in the other two patterns, the two corresponding numbers are identical (two copies of 2688, and two of 1664). If that's deliberate, could you add a comment saying why?


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1992
+                                  (v4i32 MQPR:$reg)))))),
+            (v4i32(MVE_VQABSs32 (v4i32 MQPR:$reg)))>;
+}
----------------
Since all three of these patterns look very similar, it ought to be possible to fold them all up into a class or multiclass. If you pass an `MVEVectorVTInfo` as one of the template parameters to the class, you should be able to extract both the vector type and the predicate type that goes with it (e.g. `MVE_v16i8.Vec` is `v16i8`, and `MVE_v16i8.Pred` is `v16i1`).

You might have to pass those magic integer constants as extra template parameters, though.


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

https://reviews.llvm.org/D70181