diff mbox series

match.pd: Use element_mode instead of TYPE_MODE.

Message ID 3fc809a1-6667-daca-e95a-b0a58825e16f@gmail.com
State New
Headers show
Series match.pd: Use element_mode instead of TYPE_MODE. | expand

Commit Message

Robin Dapp June 26, 2023, 2:26 p.m. UTC 
Hi,

this patch changes TYPE_MODE into element_mode in a match.pd
simplification.  As the simplification can be called with vector types
real_can_shorten_arithmetic would ICE in REAL_MODE_FORMAT which
expects a scalar mode.  Therefore, use element_mode instead of
TYPE_MODE.

Additionally, check if the target supports the resulting operation in the
new mode.  One target that supports e.g. a float addition but not a
_Float16 addition is the RISC-V vector Float16 extension Zvfhmin.

Bootstrap on x86_64 succeeded, testsuite is currently running.  Is this OK
if the testsuite is clean?

Regards
 Robin

gcc/ChangeLog:

	* match.pd: Use element_mode and check if target supports
	operation with new type.
---
 gcc/match.pd | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Jeff Law June 26, 2023, 11:18 p.m. UTC | #1 
On 6/26/23 08:26, Robin Dapp via Gcc-patches wrote:
> Hi,
> 
> this patch changes TYPE_MODE into element_mode in a match.pd
> simplification.  As the simplification can be called with vector types
> real_can_shorten_arithmetic would ICE in REAL_MODE_FORMAT which
> expects a scalar mode.  Therefore, use element_mode instead of
> TYPE_MODE.
> 
> Additionally, check if the target supports the resulting operation in the
> new mode.  One target that supports e.g. a float addition but not a
> _Float16 addition is the RISC-V vector Float16 extension Zvfhmin.
> 
> Bootstrap on x86_64 succeeded, testsuite is currently running.  Is this OK
> if the testsuite is clean?
> 
> Regards
>   Robin
> 
> gcc/ChangeLog:
> 
> 	* match.pd: Use element_mode and check if target supports
> 	operation with new type.
Ugh.  What a mess -- not helped by the fact the code is poorly indented. 
  Indention would lead one to believe this is guarded by the integral 
type test.


> +		   && target_supports_op_p (newtype, op, optab_default)
FWIW, I think there are other targets that support various 16bit FP 
modes, but not the full complement of operations.  It was fairly common 
when the 16bit FP modes were first landing.


OK after the usual bootstrap & testing.

jeff
Richard Biener June 27, 2023, 6:30 a.m. UTC | #2 
On Mon, 26 Jun 2023, Robin Dapp wrote:

> Hi,
> 
> this patch changes TYPE_MODE into element_mode in a match.pd
> simplification.  As the simplification can be called with vector types
> real_can_shorten_arithmetic would ICE in REAL_MODE_FORMAT which
> expects a scalar mode.  Therefore, use element_mode instead of
> TYPE_MODE.
> 
> Additionally, check if the target supports the resulting operation in the
> new mode.  One target that supports e.g. a float addition but not a
> _Float16 addition is the RISC-V vector Float16 extension Zvfhmin.
> 
> Bootstrap on x86_64 succeeded, testsuite is currently running.  Is this OK
> if the testsuite is clean?

The element_mode passing is OK.  We don't do a target support check
in any other place when non-vector operations are involved.

Can you push the element_mode change separately please?

I'd like to hear more reasoning of why target_supports_op_p is wanted
here.  Doesn't target_supports_op_p return false if this is for example
a soft-fp target?  So if at all, shouldn't the test only be carried
out if the original operation was supported by the target?

Thanks,
Richard.

> Regards
>  Robin
> 
> gcc/ChangeLog:
> 
> 	* match.pd: Use element_mode and check if target supports
> 	operation with new type.
> ---
>  gcc/match.pd | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 33ccda3e7b6..4a200f221f6 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -7454,10 +7454,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>  		  values representable in the TYPE to be within the
>  		  range of normal values of ITYPE.  */
>  	      (if (element_precision (newtype) < element_precision (itype)
> +		   && target_supports_op_p (newtype, op, optab_default)
>  		   && (flag_unsafe_math_optimizations
>  		       || (element_precision (newtype) == element_precision (type)
> -			   && real_can_shorten_arithmetic (TYPE_MODE (itype),
> -							   TYPE_MODE (type))
> +			   && real_can_shorten_arithmetic (element_mode (itype),
> +							   element_mode (type))
>  			   && !excess_precision_type (newtype)))
>  		   && !types_match (itype, newtype))
>  		 (convert:type (op (convert:newtype @1)
>
Robin Dapp June 27, 2023, 6:47 a.m. UTC | #3 
> Can you push the element_mode change separately please?

OK.

> I'd like to hear more reasoning of why target_supports_op_p is wanted
> here.  Doesn't target_supports_op_p return false if this is for example
> a soft-fp target?  So if at all, shouldn't the test only be carried
> out if the original operation was supported by the target?

Tamar and I discussed whether a target check is appropriate yesterday
already here and were torn.  How or where would we expect a non-supported
operation to be discarded, though?  In my case the expression is generated
during a ranger fold and survives until expand where we ICE.

Regards
 Robin
Richard Biener June 27, 2023, 7:03 a.m. UTC | #4 
On Tue, 27 Jun 2023, Robin Dapp wrote:

> > Can you push the element_mode change separately please?
> 
> OK.
> 
> > I'd like to hear more reasoning of why target_supports_op_p is wanted
> > here.  Doesn't target_supports_op_p return false if this is for example
> > a soft-fp target?  So if at all, shouldn't the test only be carried
> > out if the original operation was supported by the target?
> 
> Tamar and I discussed whether a target check is appropriate yesterday
> already here and were torn.  How or where would we expect a non-supported
> operation to be discarded, though?  In my case the expression is generated
> during a ranger fold and survives until expand where we ICE.

Why does the expander not have a fallback here?  If we put up
restrictions like this like we do for vector operations (after
vector lowering!), we need to document this.  Your check covers
more than just FP16 types as well which I think is undesirable.

So it seems for FP16 we need this for correctness (to not ICE)
while for other modes it might be appropriate for performance
(though I cannot imagine a target supporting say long double
not supporting float).

Richard.
Robin Dapp June 27, 2023, 7:42 a.m. UTC | #5 
> Why does the expander not have a fallback here?  If we put up
> restrictions like this like we do for vector operations (after
> vector lowering!), we need to document this.  Your check covers
> more than just FP16 types as well which I think is undesirable.

I'm not sure I follow.  What would we fall back to if
(_Float16)a + (_Float16)b is not supported?  Should I provide
a (_Float16)((float)a + (float)b) fallback?  But that would just
undo the simplification we performed.  Or do you mean in optabs
already?

> So it seems for FP16 we need this for correctness (to not ICE)
> while for other modes it might be appropriate for performance
> (though I cannot imagine a target supporting say long double
> not supporting float).

What about something like:

-                  && target_supports_op_p (newtype, op, optab_default)
+                  && (!target_supports_op_p (itype, op, optab_default)
+                      || element_mode (newtype) != HFmode
+                      || target_supports_op_p (newtype, op, optab_default))
?

Regards
 Robin
Richard Biener June 27, 2023, 8:46 a.m. UTC | #6 
On Tue, 27 Jun 2023, Robin Dapp wrote:

> > Why does the expander not have a fallback here?  If we put up
> > restrictions like this like we do for vector operations (after
> > vector lowering!), we need to document this.  Your check covers
> > more than just FP16 types as well which I think is undesirable.
> 
> I'm not sure I follow.  What would we fall back to if
> (_Float16)a + (_Float16)b is not supported?  Should I provide
> a (_Float16)((float)a + (float)b) fallback?  But that would just
> undo the simplification we performed.  Or do you mean in optabs
> already?

Yeah, the optab should already have the fallback of WIDENing here?
So why does that fail?

> > So it seems for FP16 we need this for correctness (to not ICE)
> > while for other modes it might be appropriate for performance
> > (though I cannot imagine a target supporting say long double
> > not supporting float).
> 
> What about something like:
> 
> -                  && target_supports_op_p (newtype, op, optab_default)
> +                  && (!target_supports_op_p (itype, op, optab_default)
> +                      || element_mode (newtype) != HFmode
> +                      || target_supports_op_p (newtype, op, optab_default))

I'd say

      && (!target_supports_op_p (itype, op, optab_default)
          || target_supports_op_p (newtype, op, optab_default))

would make sense in general.  But as said you'll likely find (many?)
other places affected.  Singling out HFmode probably doesn't work
across targets since this mode isn't defined in generic code.

Richard.
Robin Dapp June 27, 2023, 9:42 a.m. UTC | #7 
> Yeah, the optab should already have the fallback of WIDENing here?
> So why does that fail?

We reach
 if (CLASS_HAS_WIDER_MODES_P (mclass))
which returns false because mclass == MODE_VECTOR_FLOAT.
CLASS_HAS_WIDER_MODES_P only handles non-vector classes?
Same for FOR_EACH_WIDER_MODE that follows.

Regards
 Robin
Richard Biener June 27, 2023, 9:50 a.m. UTC | #8 
On Tue, 27 Jun 2023, Robin Dapp wrote:

> > Yeah, the optab should already have the fallback of WIDENing here?
> > So why does that fail?
> 
> We reach
>  if (CLASS_HAS_WIDER_MODES_P (mclass))
> which returns false because mclass == MODE_VECTOR_FLOAT.
> CLASS_HAS_WIDER_MODES_P only handles non-vector classes?
> Same for FOR_EACH_WIDER_MODE that follows.

Oh, so this is about vector modes.  So yes, for vectors we need to
perform this test.  In other places we do

        && (!VECTOR_MODE_P (TYPE_MODE (type))
            || (VECTOR_MODE_P (TYPE_MODE (itype))
                && optab_handler (and_optab,
                                  TYPE_MODE (itype)) != 
CODE_FOR_nothing)))

so I suggest to do a similar VECTOR_MODE_P check and your original test.
So

      && (!VECTOR_MODE_P (TYPE_MODE (newtype))
          || target_supports_op_p (newtype, op, optab_default))

OK with that change.

Thanks,
Richard.
Robin Dapp June 27, 2023, 9:55 a.m. UTC | #9 
> so I suggest to do a similar VECTOR_MODE_P check and your original test.
> So
> 
>       && (!VECTOR_MODE_P (TYPE_MODE (newtype))
>           || target_supports_op_p (newtype, op, optab_default))
> 
> OK with that change.

Separate patch or into the original one?  We needed element_mode because
TYPE_MODE wouldn't work for a vector_mode so it still somehow fits.

Apart from that, out of curiosity, do we want the same optab mechanism
(try widening/widened op if the original one failed) for vector types as
well in the future?

Regards
 Robin
Richard Biener June 27, 2023, 10:05 a.m. UTC | #10 
On Tue, 27 Jun 2023, Robin Dapp wrote:

> > so I suggest to do a similar VECTOR_MODE_P check and your original test.
> > So
> > 
> >       && (!VECTOR_MODE_P (TYPE_MODE (newtype))
> >           || target_supports_op_p (newtype, op, optab_default))
> > 
> > OK with that change.
> 
> Separate patch or into the original one?  We needed element_mode because
> TYPE_MODE wouldn't work for a vector_mode so it still somehow fits.

You can put it into the original one.

> Apart from that, out of curiosity, do we want the same optab mechanism
> (try widening/widened op if the original one failed) for vector types as
> well in the future?

With the current design that would belong to vector lowering.  So no,
I don't think so.

Richard.
Robin Dapp June 27, 2023, 3:55 p.m. UTC | #11 
> You can put it into the original one.

Bootstrap and testsuite run were successful.
I'm going to push the attached, thanks.

Regards
 Robin

diff --git a/gcc/match.pd b/gcc/match.pd
index 33ccda3e7b6..83bcefa914b 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -7454,10 +7454,12 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
                  values representable in the TYPE to be within the
                  range of normal values of ITYPE.  */
              (if (element_precision (newtype) < element_precision (itype)
+                  && (!VECTOR_MODE_P (TYPE_MODE (newtype))
+                      || target_supports_op_p (newtype, op, optab_default))
                   && (flag_unsafe_math_optimizations
                       || (element_precision (newtype) == element_precision (type)
-                          && real_can_shorten_arithmetic (TYPE_MODE (itype),
-                                                          TYPE_MODE (type))
+                          && real_can_shorten_arithmetic (element_mode (itype),
+                                                          element_mode (type))
                           && !excess_precision_type (newtype)))
                   && !types_match (itype, newtype))
                 (convert:type (op (convert:newtype @1)
Andrew Pinski June 27, 2023, 11:05 p.m. UTC | #12 
On Tue, Jun 27, 2023 at 8:56 AM Robin Dapp via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> > You can put it into the original one.
>
> Bootstrap and testsuite run were successful.
> I'm going to push the attached, thanks.

I am reducing a bug report which I think will be fixed by this change
(PR 110444). I will double check to see if this has fixed this issue
once I finished reducing it.
I will commit a testcase if this patch fixed the issue.

Thanks,
Andrew Pinski

>
> Regards
>  Robin
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 33ccda3e7b6..83bcefa914b 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -7454,10 +7454,12 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>                   values representable in the TYPE to be within the
>                   range of normal values of ITYPE.  */
>               (if (element_precision (newtype) < element_precision (itype)
> +                  && (!VECTOR_MODE_P (TYPE_MODE (newtype))
> +                      || target_supports_op_p (newtype, op, optab_default))
>                    && (flag_unsafe_math_optimizations
>                        || (element_precision (newtype) == element_precision (type)
> -                          && real_can_shorten_arithmetic (TYPE_MODE (itype),
> -                                                          TYPE_MODE (type))
> +                          && real_can_shorten_arithmetic (element_mode (itype),
> +                                                          element_mode (type))
>                            && !excess_precision_type (newtype)))
>                    && !types_match (itype, newtype))
>                  (convert:type (op (convert:newtype @1)
>
Andrew Pinski June 28, 2023, 7:27 a.m. UTC | #13 
On Tue, Jun 27, 2023 at 4:05 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Tue, Jun 27, 2023 at 8:56 AM Robin Dapp via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > > You can put it into the original one.
> >
> > Bootstrap and testsuite run were successful.
> > I'm going to push the attached, thanks.
>
> I am reducing a bug report which I think will be fixed by this change
> (PR 110444). I will double check to see if this has fixed this issue
> once I finished reducing it.
> I will commit a testcase if this patch fixed the issue.

Yes it was fixed by this change. Committed the testcase as
r14-2151-g857e1f93ff8e3b93a7a3dc .

Thanks,
Andrew Pinski

>
> Thanks,
> Andrew Pinski
>
> >
> > Regards
> >  Robin
> >
> > diff --git a/gcc/match.pd b/gcc/match.pd
> > index 33ccda3e7b6..83bcefa914b 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -7454,10 +7454,12 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> >                   values representable in the TYPE to be within the
> >                   range of normal values of ITYPE.  */
> >               (if (element_precision (newtype) < element_precision (itype)
> > +                  && (!VECTOR_MODE_P (TYPE_MODE (newtype))
> > +                      || target_supports_op_p (newtype, op, optab_default))
> >                    && (flag_unsafe_math_optimizations
> >                        || (element_precision (newtype) == element_precision (type)
> > -                          && real_can_shorten_arithmetic (TYPE_MODE (itype),
> > -                                                          TYPE_MODE (type))
> > +                          && real_can_shorten_arithmetic (element_mode (itype),
> > +                                                          element_mode (type))
> >                            && !excess_precision_type (newtype)))
> >                    && !types_match (itype, newtype))
> >                  (convert:type (op (convert:newtype @1)
> >
diff mbox series

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index 33ccda3e7b6..4a200f221f6 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -7454,10 +7454,11 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 		  values representable in the TYPE to be within the
 		  range of normal values of ITYPE.  */
 	      (if (element_precision (newtype) < element_precision (itype)
+		   && target_supports_op_p (newtype, op, optab_default)
 		   && (flag_unsafe_math_optimizations
 		       || (element_precision (newtype) == element_precision (type)
-			   && real_can_shorten_arithmetic (TYPE_MODE (itype),
-							   TYPE_MODE (type))
+			   && real_can_shorten_arithmetic (element_mode (itype),
+							   element_mode (type))
 			   && !excess_precision_type (newtype)))
 		   && !types_match (itype, newtype))
 		 (convert:type (op (convert:newtype @1)