[PATCH] RS6000 Add testlsbb (Test LSB by Byte) operations

[Segher Boessenkool]

Hi!

On Tue, Jul 28, 2020 at 10:31:02AM -0500, will schmidt wrote:
> >     * config/rs6000/rs6000-builtin.def (BU_FUTURE_VSX_1): New built-
> > in
> >     handling macro.  (XVTLSBB_ZEROS, XVTLSBB_ONES) New builtin
> > defines.

New (...) should start a new line.  Missing colon here, btw.

> >     (xvtlsbb_zeros, xvtlsbb_ones) New builtin overloads.

And here.

> > --- a/gcc/config/rs6000/altivec.h
> > +++ b/gcc/config/rs6000/altivec.h
> > @@ -490,10 +490,13 @@
> >  #define vec_cmpnez __builtin_vec_vcmpnez
> > 
> >  #define vec_cntlz_lsbb __builtin_vec_vclzlsbb
> >  #define vec_cnttz_lsbb __builtin_vec_vctzlsbb
> > 
> > +#define vec_test_lsbb_all_ones __builtin_vec_xvtlsbb_ones
> > +#define vec_test_lsbb_all_zeros __builtin_vec_xvtlsbb_zeros

Maybe the builtins should have "_all" in the name as well?  It seems
clearer, and it's not much more to type (or read).

> > +BU_FUTURE_OVERLOAD_1 (XVTLSBB_ZEROS, "xvtlsbb_zeros")
> > +BU_FUTURE_OVERLOAD_1 (XVTLSBB_ONES,  "xvtlsbb_ones")

Here too, then.

> > +;; xvtlsbb BF,XB
> > +;; set CR field BF to indicate if bit 7 of every byte element in
> > VSR[XB}

Capital S in Set.  And you typoed the ].

"The low bit of every byte", perhaps?

> > +;; is equal to 1 (ALL_TRUE) or equal to 0 (ALL_FALSE).
> > +;; CR.field_WRITE (BF, ALL_TRUE, 0, ALL_FALSE, 0);

That last line is copied from the ISA, but is a bit cryptical without
context.

> > +(define_insn "*xvtlsbb_internal"
> > +  [(set (match_operand:CC 0 "cc_reg_operand" "=y")
> > +	(unspec:CC [(match_operand:V16QI 1 "vsx_register_operand"
> > "wa")]

The line is wrapped strangely here.  Maybe it is how you mailed it?  But
please check.

> > +	 UNSPEC_XVTLSBB))]
> > +"TARGET_FUTURE"
> > +"xvtlsbb %0,%x1"
> > +[(set_attr "type" "logical")])

And here it is indented wrong.

> > +(define_expand "xvtlsbbo"
> > +  [(set (match_dup 2)
> > +	(unspec:CC [(match_operand:V16QI 1 "vsx_register_operand" "v")]
> > +	 UNSPEC_XVTLSBB))
> > +	(set (match_operand:SI 0 "gpc_reg_operand" "=r")
> > +		(lt:SI (match_dup 2) (const_int 0)))]
> > +"TARGET_FUTURE"
> > +{
> > +operands[2] = gen_reg_rtx (CCmode);
> > +})
> > +(define_expand "xvtlsbbz"
> > +  [(set (match_dup 2)
> > +	(unspec:CC [(match_operand:V16QI 1 "vsx_register_operand" "v")]
> > +	 UNSPEC_XVTLSBB))
> > +	(set (match_operand:SI 0 "gpc_reg_operand" "=r")
> > +		(eq:SI (match_dup 2) (const_int 0)))]
> > +"TARGET_FUTURE"
> > +{
> > +operands[2] = gen_reg_rtx (CCmode);
> > +})

All of this as well.

> > +/* { dg-do run } */
> > +/* { dg-require-effective-target powerpc_future_hw } */
> > +/* { dg-options "-O2 -fno-inline -mvsx -mdejagnu-cpu=future" } */

-mvsx is implied by -mcpu=power10.

> > +/* { dg-final { scan-assembler-times {\mxvtlsbb\M} 2 } } */
> > +/* { dg-final { scan-assembler-times {\msetbc\M} 2 } } */

So I wonder if it can always get the correct bit (we do not have CC modes
that allow multiple bits to be set, but for this particular insn, it
never *can* set multiple bits!)


Should be okay with those things fixed (and that _all thought about).
Okay for trunk, or you can send one more version if you prefer :-)

Thanks!


Segher