Re: [PATCH] vect: Use xor to invert oversized vector masks
On Fri, Mar 15, 2024 at 12:24 PM Andrew Stubbs wrote:
>
> On 15/03/2024 07:35, Richard Biener wrote:
> > On Fri, Mar 15, 2024 at 4:35 AM Hongtao Liu wrote:
> >>
> >> On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs wrote:
> >>>
> >>> Don't enable excess lanes when inverting vector bit-masks smaller than the
> >>> integer mode. This is yet another case of wrong-code due to mishandling
> >>> of oversized bitmasks.
> >>>
> >>> This issue shows up in vect/tsvc/vect-tsvc-s278.c and
> >>> vect/tsvc/vect-tsvc-s279.c if I set the preferred vector size to V32
> >>> (down from V64) on amdgcn.
> >>>
> >>> OK for mainline?
> >>>
> >>> Andrew
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> * expr.cc (expand_expr_real_2): Use xor to invert vector masks.
> >>> ---
> >>> gcc/expr.cc | 11 +++
> >>> 1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/gcc/expr.cc b/gcc/expr.cc
> >>> index 403eeaa108e4..3540327d879e 100644
> >>> --- a/gcc/expr.cc
> >>> +++ b/gcc/expr.cc
> >>> @@ -10497,6 +10497,17 @@ expand_expr_real_2 (sepops ops, rtx target,
> >>> machine_mode tmode,
> >>> immed_wide_int_const (mask, int_mode),
> >>> target, 1, OPTAB_LIB_WIDEN);
> >>> }
> >>> + /* If it's a vector mask don't enable excess bits. */
> >>> + else if (VECTOR_BOOLEAN_TYPE_P (type)
> >>> + && SCALAR_INT_MODE_P (mode)
> >>> + && maybe_ne (GET_MODE_PRECISION (mode),
> >>> + TYPE_VECTOR_SUBPARTS (type).to_constant ()))
> >>> + {
> >>> + auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
> >>> + temp = expand_binop (mode, xor_optab, op0,
> >>> + GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
> >>> + target, true, OPTAB_WIDEN);
> >>> + }
> >> Not review, just curious, should the issue be fixed by the commit in
> >> PR113576.
> >> Also wonder besides cbranch, excess land bits also matter?
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113576#c35
> >
> > Yes, you patch BIT_NOT but we decided to patch final compares. Is it that
> > we need to fixup every mask use in a .COND_* expansion as well? If so
> > we should do it there.
>
> I thought that the "not" to "xor" change was nice and there was already
> code there for fixing bitfields, but OK, I take your point.
>
> The .COND_* statements are handled as internal function calls that are
> expanded directly via the optab with no special cases for different call
> types. This is because the "expand_cond_*_optab_fn" functions just map
> straight to "expand_direct_optab_fn" would that be the right place
> to insert a special case handler to insert "and" operations?
Yes, I think in expand_fn_using_insn where we handle the "undefined" input
operands we want to handle the vector bool operands as well, masking
rhs_rtx accordingly.
Richard.
> Andrew
Re: [PATCH] vect: Use xor to invert oversized vector masks
On 15/03/2024 07:35, Richard Biener wrote:
On Fri, Mar 15, 2024 at 4:35 AM Hongtao Liu wrote:
On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs wrote:
Don't enable excess lanes when inverting vector bit-masks smaller than the
integer mode. This is yet another case of wrong-code due to mishandling
of oversized bitmasks.
This issue shows up in vect/tsvc/vect-tsvc-s278.c and
vect/tsvc/vect-tsvc-s279.c if I set the preferred vector size to V32
(down from V64) on amdgcn.
OK for mainline?
Andrew
gcc/ChangeLog:
* expr.cc (expand_expr_real_2): Use xor to invert vector masks.
---
gcc/expr.cc | 11 +++
1 file changed, 11 insertions(+)
diff --git a/gcc/expr.cc b/gcc/expr.cc
index 403eeaa108e4..3540327d879e 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -10497,6 +10497,17 @@ expand_expr_real_2 (sepops ops, rtx target,
machine_mode tmode,
immed_wide_int_const (mask, int_mode),
target, 1, OPTAB_LIB_WIDEN);
}
+ /* If it's a vector mask don't enable excess bits. */
+ else if (VECTOR_BOOLEAN_TYPE_P (type)
+ && SCALAR_INT_MODE_P (mode)
+ && maybe_ne (GET_MODE_PRECISION (mode),
+ TYPE_VECTOR_SUBPARTS (type).to_constant ()))
+ {
+ auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
+ temp = expand_binop (mode, xor_optab, op0,
+ GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
+ target, true, OPTAB_WIDEN);
+ }
Not review, just curious, should the issue be fixed by the commit in PR113576.
Also wonder besides cbranch, excess land bits also matter?
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113576#c35
Yes, you patch BIT_NOT but we decided to patch final compares. Is it that
we need to fixup every mask use in a .COND_* expansion as well? If so
we should do it there.
I thought that the "not" to "xor" change was nice and there was already
code there for fixing bitfields, but OK, I take your point.
The .COND_* statements are handled as internal function calls that are
expanded directly via the optab with no special cases for different call
types. This is because the "expand_cond_*_optab_fn" functions just map
straight to "expand_direct_optab_fn" would that be the right place
to insert a special case handler to insert "and" operations?
Andrew
Re: [PATCH] vect: Use xor to invert oversized vector masks
On 15/03/2024 03:45, Hongtao Liu wrote:
On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs wrote:
Don't enable excess lanes when inverting vector bit-masks smaller than the
integer mode. This is yet another case of wrong-code due to mishandling
of oversized bitmasks.
This issue shows up in vect/tsvc/vect-tsvc-s278.c and
vect/tsvc/vect-tsvc-s279.c if I set the preferred vector size to V32
(down from V64) on amdgcn.
OK for mainline?
Andrew
gcc/ChangeLog:
* expr.cc (expand_expr_real_2): Use xor to invert vector masks.
---
gcc/expr.cc | 11 +++
1 file changed, 11 insertions(+)
diff --git a/gcc/expr.cc b/gcc/expr.cc
index 403eeaa108e4..3540327d879e 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -10497,6 +10497,17 @@ expand_expr_real_2 (sepops ops, rtx target,
machine_mode tmode,
immed_wide_int_const (mask, int_mode),
target, 1, OPTAB_LIB_WIDEN);
}
+ /* If it's a vector mask don't enable excess bits. */
+ else if (VECTOR_BOOLEAN_TYPE_P (type)
+ && SCALAR_INT_MODE_P (mode)
+ && maybe_ne (GET_MODE_PRECISION (mode),
+ TYPE_VECTOR_SUBPARTS (type).to_constant ()))
+ {
+ auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
+ temp = expand_binop (mode, xor_optab, op0,
+ GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
+ target, true, OPTAB_WIDEN);
+ }
Not review, just curious, should the issue be fixed by the commit in PR113576.
Also wonder besides cbranch, excess land bits also matter?
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113576#c35
It does seem to be another case of the same problem, but those commits
are long enough ago that I do have them, and still saw a problem.
Andrew
Re: [PATCH] vect: Use xor to invert oversized vector masks
On Fri, Mar 15, 2024 at 4:35 AM Hongtao Liu wrote:
>
> On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs wrote:
> >
> > Don't enable excess lanes when inverting vector bit-masks smaller than the
> > integer mode. This is yet another case of wrong-code due to mishandling
> > of oversized bitmasks.
> >
> > This issue shows up in vect/tsvc/vect-tsvc-s278.c and
> > vect/tsvc/vect-tsvc-s279.c if I set the preferred vector size to V32
> > (down from V64) on amdgcn.
> >
> > OK for mainline?
> >
> > Andrew
> >
> > gcc/ChangeLog:
> >
> > * expr.cc (expand_expr_real_2): Use xor to invert vector masks.
> > ---
> > gcc/expr.cc | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/gcc/expr.cc b/gcc/expr.cc
> > index 403eeaa108e4..3540327d879e 100644
> > --- a/gcc/expr.cc
> > +++ b/gcc/expr.cc
> > @@ -10497,6 +10497,17 @@ expand_expr_real_2 (sepops ops, rtx target,
> > machine_mode tmode,
> >immed_wide_int_const (mask, int_mode),
> >target, 1, OPTAB_LIB_WIDEN);
> > }
> > + /* If it's a vector mask don't enable excess bits. */
> > + else if (VECTOR_BOOLEAN_TYPE_P (type)
> > + && SCALAR_INT_MODE_P (mode)
> > + && maybe_ne (GET_MODE_PRECISION (mode),
> > + TYPE_VECTOR_SUBPARTS (type).to_constant ()))
> > + {
> > + auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
> > + temp = expand_binop (mode, xor_optab, op0,
> > + GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
> > + target, true, OPTAB_WIDEN);
> > + }
> Not review, just curious, should the issue be fixed by the commit in PR113576.
> Also wonder besides cbranch, excess land bits also matter?
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113576#c35
Yes, you patch BIT_NOT but we decided to patch final compares. Is it that
we need to fixup every mask use in a .COND_* expansion as well? If so
we should do it there.
Richard.
> >else
> > temp = expand_unop (mode, one_cmpl_optab, op0, target, 1);
> >gcc_assert (temp);
> > --
> > 2.41.0
> >
>
>
> --
> BR,
> Hongtao
Re: [PATCH] vect: Use xor to invert oversized vector masks
On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs wrote:
>
> Don't enable excess lanes when inverting vector bit-masks smaller than the
> integer mode. This is yet another case of wrong-code due to mishandling
> of oversized bitmasks.
>
> This issue shows up in vect/tsvc/vect-tsvc-s278.c and
> vect/tsvc/vect-tsvc-s279.c if I set the preferred vector size to V32
> (down from V64) on amdgcn.
>
> OK for mainline?
>
> Andrew
>
> gcc/ChangeLog:
>
> * expr.cc (expand_expr_real_2): Use xor to invert vector masks.
> ---
> gcc/expr.cc | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 403eeaa108e4..3540327d879e 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -10497,6 +10497,17 @@ expand_expr_real_2 (sepops ops, rtx target,
> machine_mode tmode,
>immed_wide_int_const (mask, int_mode),
>target, 1, OPTAB_LIB_WIDEN);
> }
> + /* If it's a vector mask don't enable excess bits. */
> + else if (VECTOR_BOOLEAN_TYPE_P (type)
> + && SCALAR_INT_MODE_P (mode)
> + && maybe_ne (GET_MODE_PRECISION (mode),
> + TYPE_VECTOR_SUBPARTS (type).to_constant ()))
> + {
> + auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
> + temp = expand_binop (mode, xor_optab, op0,
> + GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
> + target, true, OPTAB_WIDEN);
> + }
Not review, just curious, should the issue be fixed by the commit in PR113576.
Also wonder besides cbranch, excess land bits also matter?
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113576#c35
>else
> temp = expand_unop (mode, one_cmpl_optab, op0, target, 1);
>gcc_assert (temp);
> --
> 2.41.0
>
--
BR,
Hongtao
[PATCH] vect: Use xor to invert oversized vector masks
Don't enable excess lanes when inverting vector bit-masks smaller than the
integer mode. This is yet another case of wrong-code due to mishandling
of oversized bitmasks.
This issue shows up in vect/tsvc/vect-tsvc-s278.c and
vect/tsvc/vect-tsvc-s279.c if I set the preferred vector size to V32
(down from V64) on amdgcn.
OK for mainline?
Andrew
gcc/ChangeLog:
* expr.cc (expand_expr_real_2): Use xor to invert vector masks.
---
gcc/expr.cc | 11 +++
1 file changed, 11 insertions(+)
diff --git a/gcc/expr.cc b/gcc/expr.cc
index 403eeaa108e4..3540327d879e 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -10497,6 +10497,17 @@ expand_expr_real_2 (sepops ops, rtx target,
machine_mode tmode,
immed_wide_int_const (mask, int_mode),
target, 1, OPTAB_LIB_WIDEN);
}
+ /* If it's a vector mask don't enable excess bits. */
+ else if (VECTOR_BOOLEAN_TYPE_P (type)
+ && SCALAR_INT_MODE_P (mode)
+ && maybe_ne (GET_MODE_PRECISION (mode),
+ TYPE_VECTOR_SUBPARTS (type).to_constant ()))
+ {
+ auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
+ temp = expand_binop (mode, xor_optab, op0,
+ GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
+ target, true, OPTAB_WIDEN);
+ }
else
temp = expand_unop (mode, one_cmpl_optab, op0, target, 1);
gcc_assert (temp);
--
2.41.0
