* [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
@ 2023-06-26 14:26 Robin Dapp
2023-06-26 23:18 ` Jeff Law
2023-06-27 6:30 ` Richard Biener
0 siblings, 2 replies; 14+ messages in thread
From: Robin Dapp @ 2023-06-26 14:26 UTC (permalink / raw)
To: gcc-patches; +Cc: rdapp.gcc, Richard Biener, Tamar.Christina
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(-)
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)
--
2.41.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-26 14:26 [PATCH] match.pd: Use element_mode instead of TYPE_MODE Robin Dapp
@ 2023-06-26 23:18 ` Jeff Law
2023-06-27 6:30 ` Richard Biener
1 sibling, 0 replies; 14+ messages in thread
From: Jeff Law @ 2023-06-26 23:18 UTC (permalink / raw)
To: Robin Dapp, gcc-patches; +Cc: Richard Biener, Tamar.Christina
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-26 14:26 [PATCH] match.pd: Use element_mode instead of TYPE_MODE Robin Dapp
2023-06-26 23:18 ` Jeff Law
@ 2023-06-27 6:30 ` Richard Biener
2023-06-27 6:47 ` Robin Dapp
1 sibling, 1 reply; 14+ messages in thread
From: Richard Biener @ 2023-06-27 6:30 UTC (permalink / raw)
To: Robin Dapp; +Cc: gcc-patches, Tamar.Christina
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)
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 6:30 ` Richard Biener
@ 2023-06-27 6:47 ` Robin Dapp
2023-06-27 7:03 ` Richard Biener
0 siblings, 1 reply; 14+ messages in thread
From: Robin Dapp @ 2023-06-27 6:47 UTC (permalink / raw)
To: Richard Biener; +Cc: rdapp.gcc, gcc-patches, Tamar.Christina
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 6:47 ` Robin Dapp
@ 2023-06-27 7:03 ` Richard Biener
2023-06-27 7:42 ` Robin Dapp
0 siblings, 1 reply; 14+ messages in thread
From: Richard Biener @ 2023-06-27 7:03 UTC (permalink / raw)
To: Robin Dapp; +Cc: gcc-patches, Tamar.Christina
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 7:03 ` Richard Biener
@ 2023-06-27 7:42 ` Robin Dapp
2023-06-27 8:46 ` Richard Biener
0 siblings, 1 reply; 14+ messages in thread
From: Robin Dapp @ 2023-06-27 7:42 UTC (permalink / raw)
To: Richard Biener; +Cc: rdapp.gcc, gcc-patches, Tamar.Christina
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 7:42 ` Robin Dapp
@ 2023-06-27 8:46 ` Richard Biener
2023-06-27 9:42 ` Robin Dapp
0 siblings, 1 reply; 14+ messages in thread
From: Richard Biener @ 2023-06-27 8:46 UTC (permalink / raw)
To: Robin Dapp; +Cc: gcc-patches, Tamar.Christina
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 8:46 ` Richard Biener
@ 2023-06-27 9:42 ` Robin Dapp
2023-06-27 9:50 ` Richard Biener
0 siblings, 1 reply; 14+ messages in thread
From: Robin Dapp @ 2023-06-27 9:42 UTC (permalink / raw)
To: Richard Biener; +Cc: rdapp.gcc, gcc-patches, Tamar.Christina
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 9:42 ` Robin Dapp
@ 2023-06-27 9:50 ` Richard Biener
2023-06-27 9:55 ` Robin Dapp
0 siblings, 1 reply; 14+ messages in thread
From: Richard Biener @ 2023-06-27 9:50 UTC (permalink / raw)
To: Robin Dapp; +Cc: gcc-patches, Tamar.Christina
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 9:50 ` Richard Biener
@ 2023-06-27 9:55 ` Robin Dapp
2023-06-27 10:05 ` Richard Biener
0 siblings, 1 reply; 14+ messages in thread
From: Robin Dapp @ 2023-06-27 9:55 UTC (permalink / raw)
To: Richard Biener; +Cc: rdapp.gcc, gcc-patches, Tamar.Christina
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 9:55 ` Robin Dapp
@ 2023-06-27 10:05 ` Richard Biener
2023-06-27 15:55 ` Robin Dapp
0 siblings, 1 reply; 14+ messages in thread
From: Richard Biener @ 2023-06-27 10:05 UTC (permalink / raw)
To: Robin Dapp; +Cc: gcc-patches, Tamar.Christina
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 10:05 ` Richard Biener
@ 2023-06-27 15:55 ` Robin Dapp
2023-06-27 23:05 ` Andrew Pinski
0 siblings, 1 reply; 14+ messages in thread
From: Robin Dapp @ 2023-06-27 15:55 UTC (permalink / raw)
To: Richard Biener; +Cc: rdapp.gcc, gcc-patches, Tamar.Christina
> 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 15:55 ` Robin Dapp
@ 2023-06-27 23:05 ` Andrew Pinski
2023-06-28 7:27 ` Andrew Pinski
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Pinski @ 2023-06-27 23:05 UTC (permalink / raw)
To: Robin Dapp; +Cc: Richard Biener, gcc-patches, Tamar.Christina
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)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] match.pd: Use element_mode instead of TYPE_MODE.
2023-06-27 23:05 ` Andrew Pinski
@ 2023-06-28 7:27 ` Andrew Pinski
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Pinski @ 2023-06-28 7:27 UTC (permalink / raw)
To: Robin Dapp; +Cc: Richard Biener, gcc-patches, Tamar.Christina
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)
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-06-28 7:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-26 14:26 [PATCH] match.pd: Use element_mode instead of TYPE_MODE Robin Dapp
2023-06-26 23:18 ` Jeff Law
2023-06-27 6:30 ` Richard Biener
2023-06-27 6:47 ` Robin Dapp
2023-06-27 7:03 ` Richard Biener
2023-06-27 7:42 ` Robin Dapp
2023-06-27 8:46 ` Richard Biener
2023-06-27 9:42 ` Robin Dapp
2023-06-27 9:50 ` Richard Biener
2023-06-27 9:55 ` Robin Dapp
2023-06-27 10:05 ` Richard Biener
2023-06-27 15:55 ` Robin Dapp
2023-06-27 23:05 ` Andrew Pinski
2023-06-28 7:27 ` Andrew Pinski
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).