public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] vect: Use xor to invert oversized vector masks
@ 2024-03-14 15:41 Andrew Stubbs
  2024-03-15  3:45 ` Hongtao Liu
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Stubbs @ 2024-03-14 15:41 UTC (permalink / raw)
  To: gcc-patches

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vect: Use xor to invert oversized vector masks
  2024-03-14 15:41 [PATCH] vect: Use xor to invert oversized vector masks Andrew Stubbs
@ 2024-03-15  3:45 ` Hongtao Liu
  2024-03-15  7:35   ` Richard Biener
  2024-03-15  9:52   ` Andrew Stubbs
  0 siblings, 2 replies; 6+ messages in thread
From: Hongtao Liu @ 2024-03-15  3:45 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: gcc-patches

On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs <ams@baylibre.com> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vect: Use xor to invert oversized vector masks
  2024-03-15  3:45 ` Hongtao Liu
@ 2024-03-15  7:35   ` Richard Biener
  2024-03-15 11:24     ` Andrew Stubbs
  2024-03-15  9:52   ` Andrew Stubbs
  1 sibling, 1 reply; 6+ messages in thread
From: Richard Biener @ 2024-03-15  7:35 UTC (permalink / raw)
  To: Hongtao Liu; +Cc: Andrew Stubbs, gcc-patches

On Fri, Mar 15, 2024 at 4:35 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs <ams@baylibre.com> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vect: Use xor to invert oversized vector masks
  2024-03-15  3:45 ` Hongtao Liu
  2024-03-15  7:35   ` Richard Biener
@ 2024-03-15  9:52   ` Andrew Stubbs
  1 sibling, 0 replies; 6+ messages in thread
From: Andrew Stubbs @ 2024-03-15  9:52 UTC (permalink / raw)
  To: Hongtao Liu; +Cc: gcc-patches

On 15/03/2024 03:45, Hongtao Liu wrote:
> On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs <ams@baylibre.com> 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vect: Use xor to invert oversized vector masks
  2024-03-15  7:35   ` Richard Biener
@ 2024-03-15 11:24     ` Andrew Stubbs
  2024-03-15 13:10       ` Richard Biener
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Stubbs @ 2024-03-15 11:24 UTC (permalink / raw)
  To: Richard Biener, Hongtao Liu; +Cc: gcc-patches

On 15/03/2024 07:35, Richard Biener wrote:
> On Fri, Mar 15, 2024 at 4:35 AM Hongtao Liu <crazylht@gmail.com> wrote:
>>
>> On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs <ams@baylibre.com> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] vect: Use xor to invert oversized vector masks
  2024-03-15 11:24     ` Andrew Stubbs
@ 2024-03-15 13:10       ` Richard Biener
  0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2024-03-15 13:10 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: Hongtao Liu, gcc-patches

On Fri, Mar 15, 2024 at 12:24 PM Andrew Stubbs <ams@baylibre.com> wrote:
>
> On 15/03/2024 07:35, Richard Biener wrote:
> > On Fri, Mar 15, 2024 at 4:35 AM Hongtao Liu <crazylht@gmail.com> wrote:
> >>
> >> On Thu, Mar 14, 2024 at 11:42 PM Andrew Stubbs <ams@baylibre.com> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-03-15 13:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-14 15:41 [PATCH] vect: Use xor to invert oversized vector masks Andrew Stubbs
2024-03-15  3:45 ` Hongtao Liu
2024-03-15  7:35   ` Richard Biener
2024-03-15 11:24     ` Andrew Stubbs
2024-03-15 13:10       ` Richard Biener
2024-03-15  9:52   ` Andrew Stubbs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).