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
