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

[Anna Welker via Phabricator via llvm-commits]

anwel marked 10 inline comments as done.
anwel added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1955
 
+let Predicates = [HasMVEInt] in {
+  def : Pat<(v16i8 (vselect
----------------
SjoerdMeijer wrote:
> simon_tatham wrote:
> > 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.
> Ah yes, but think I got confused by the 
> 
>    ExecuteFPCheck();
> 
> in the pseudo-code of the instruction. And not that it really matters, but I guess that means the test just needs `-mattr=+mve` instead of  `-mattr=+mve.fp` 
> 
> 
Thanks for making me aware of that, I removed the superfluous `.fp`


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1961
+                                (v16i1 (ARMvcmp (v16i8 MQPR:$reg),
+                                                (v16i8 (ARMvmovImm (i32 3712))),
+                                                (i32 0))),
----------------
simon_tatham wrote:
> 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?
A very good suggestion as this file needs more explanatory comments in it, I've added some now.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1963
+                                                (i32 0))),
+                                (v16i8 (ARMvmovImm (i32 3711))),
+                                (sub
----------------
simon_tatham wrote:
> 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?
The difference emerges because the `s16` and `s32` variants used once `ARMvmovImm ...` to represent `INT_MIN` and once `ARMvmvnImm ...` to represent `INT_MAX` (by negating `INT_MIN`), while `s8` uses `ARMvmovImm ...` both times and thus in the used encoding has to decrease the magic number for `INT_MIN` by one to represent `INT_MAX`.
In the new version of the patch this should be hopefully be more clear.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1992
+                                  (v4i32 MQPR:$reg)))))),
+            (v4i32(MVE_VQABSs32 (v4i32 MQPR:$reg)))>;
+}
----------------
simon_tatham wrote:
> 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.
Just updated the patch to do exactly this, it indeed looks much nicer.


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

https://reviews.llvm.org/D70181