* [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]
@ 2023-11-24 8:15 Jakub Jelinek
2023-11-24 9:20 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2023-11-24 8:15 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
Hi!
The bitint lowering pass only does something if it sees BITINT_TYPE (medium,
large, huge) SSA_NAMEs. In the past I've already ran into one special case
where the above doesn't work well, if there is a store of medium/large/huge
BITINT_TYPE INTEGER_CST into memory, there might not be any BITINT_TYPE
SSA_NAMEs in the function, yet we need to lower. This has been solved by
also checking for SSA_NAME_IS_VIRTUAL_OPERAND if at the vdef there isn't
such a store (the whole intent is make the pass as cheap as possible in the
currently very likely case that the IL doesn't have any BITINT_TYPEs at
all).
And the following testcase shows a similar problem. With -frounding-math
we don't fold some of FLOAT_EXPRs with INTEGER_CST operands, and if those
INTEGER_CSTs are medium/large/huge BITINT_TYPEs, we need to either cast
the INTEGER_CST to corresponding INTEGER_TYPE (for medium) or lower to
internal fn call which is later turned into libgcc call (for large/huge).
The following patch does that, but of course admittedly this discovery
of stores and FLOAT_EXPRs means we already look through quite a few
SSA_NAME_DEF_STMTs even when BITINT_TYPEs never appear.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2023-11-23 Jakub Jelinek <jakub@redhat.com>
PR middle-end/112679
* gimple-lower-bitint.cc (gimple_lower_bitint): Also stop first loop on
floating point SSA_NAME set in FLOAT_EXPR assignment from BITINT_TYPE
INTEGER_CST. Set has_large_huge for those if that BITINT_TYPE is large
or huge. Set kind to such FLOAT_EXPR assignment rhs1 BITINT_TYPE's kind.
* gcc.dg/bitint-42.c: New test.
--- gcc/gimple-lower-bitint.cc.jj 2023-11-23 14:30:02.830662509 +0100
+++ gcc/gimple-lower-bitint.cc 2023-11-23 16:17:59.217298778 +0100
@@ -5635,6 +5635,21 @@ gimple_lower_bitint (void)
break;
}
}
+ /* Similarly, e.g. with -frounding-math casts from _BitInt INTEGER_CSTs
+ to floating point types need to be rewritten. */
+ else if (SCALAR_FLOAT_TYPE_P (type))
+ {
+ gimple *g = SSA_NAME_DEF_STMT (s);
+ if (is_gimple_assign (g) && gimple_assign_rhs_code (g) == FLOAT_EXPR)
+ {
+ tree t = gimple_assign_rhs1 (g);
+ if (TREE_CODE (t) == INTEGER_CST
+ && TREE_CODE (TREE_TYPE (t)) == BITINT_TYPE
+ && (bitint_precision_kind (TREE_TYPE (t))
+ != bitint_prec_small))
+ break;
+ }
+ }
}
if (i == num_ssa_names)
return 0;
@@ -5855,6 +5870,21 @@ gimple_lower_bitint (void)
has_large_huge = true;
}
}
+ /* Similarly, e.g. with -frounding-math casts from _BitInt INTEGER_CSTs
+ to floating point types need to be rewritten. */
+ else if (SCALAR_FLOAT_TYPE_P (type))
+ {
+ gimple *g = SSA_NAME_DEF_STMT (s);
+ if (is_gimple_assign (g) && gimple_assign_rhs_code (g) == FLOAT_EXPR)
+ {
+ tree t = gimple_assign_rhs1 (g);
+ if (TREE_CODE (t) == INTEGER_CST
+ && TREE_CODE (TREE_TYPE (t)) == BITINT_TYPE
+ && (bitint_precision_kind (TREE_TYPE (t))
+ >= bitint_prec_large))
+ has_large_huge = true;
+ }
+ }
}
for (i = first_large_huge; i < num_ssa_names; ++i)
{
@@ -6182,6 +6212,19 @@ gimple_lower_bitint (void)
{
bitint_prec_kind this_kind
= bitint_precision_kind (TREE_TYPE (t));
+ if (this_kind > kind)
+ kind = this_kind;
+ }
+ }
+ if (is_gimple_assign (stmt)
+ && gimple_assign_rhs_code (stmt) == FLOAT_EXPR)
+ {
+ t = gimple_assign_rhs1 (stmt);
+ if (TREE_CODE (TREE_TYPE (t)) == BITINT_TYPE
+ && TREE_CODE (t) == INTEGER_CST)
+ {
+ bitint_prec_kind this_kind
+ = bitint_precision_kind (TREE_TYPE (t));
if (this_kind > kind)
kind = this_kind;
}
--- gcc/testsuite/gcc.dg/bitint-42.c.jj 2023-11-23 16:50:52.392502318 +0100
+++ gcc/testsuite/gcc.dg/bitint-42.c 2023-11-23 16:42:08.559881704 +0100
@@ -0,0 +1,9 @@
+/* PR middle-end/112679 */
+/* { dg-do compile { target bitint } } */
+/* { dg-options "-frounding-math" } */
+
+float
+foo (void)
+{
+ return 0x353eab28b46b03ea99b84f9736cd8dbe5e986915a0383c3cb381c0da41e31b3621c75fd53262bfcb1b0e6251dbf00f3988784e29b08b65640c263e4d0959832a20e2ff5245be1e60uwb;
+}
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]
2023-11-24 8:15 [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679] Jakub Jelinek
@ 2023-11-24 9:20 ` Richard Biener
2023-11-24 10:02 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2023-11-24 9:20 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Fri, 24 Nov 2023, Jakub Jelinek wrote:
> Hi!
>
> The bitint lowering pass only does something if it sees BITINT_TYPE (medium,
> large, huge) SSA_NAMEs. In the past I've already ran into one special case
> where the above doesn't work well, if there is a store of medium/large/huge
> BITINT_TYPE INTEGER_CST into memory, there might not be any BITINT_TYPE
> SSA_NAMEs in the function, yet we need to lower. This has been solved by
> also checking for SSA_NAME_IS_VIRTUAL_OPERAND if at the vdef there isn't
> such a store (the whole intent is make the pass as cheap as possible in the
> currently very likely case that the IL doesn't have any BITINT_TYPEs at
> all).
> And the following testcase shows a similar problem. With -frounding-math
> we don't fold some of FLOAT_EXPRs with INTEGER_CST operands, and if those
> INTEGER_CSTs are medium/large/huge BITINT_TYPEs, we need to either cast
> the INTEGER_CST to corresponding INTEGER_TYPE (for medium) or lower to
> internal fn call which is later turned into libgcc call (for large/huge).
> The following patch does that, but of course admittedly this discovery
> of stores and FLOAT_EXPRs means we already look through quite a few
> SSA_NAME_DEF_STMTs even when BITINT_TYPEs never appear.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2023-11-23 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/112679
> * gimple-lower-bitint.cc (gimple_lower_bitint): Also stop first loop on
> floating point SSA_NAME set in FLOAT_EXPR assignment from BITINT_TYPE
> INTEGER_CST. Set has_large_huge for those if that BITINT_TYPE is large
> or huge. Set kind to such FLOAT_EXPR assignment rhs1 BITINT_TYPE's kind.
>
> * gcc.dg/bitint-42.c: New test.
>
> --- gcc/gimple-lower-bitint.cc.jj 2023-11-23 14:30:02.830662509 +0100
> +++ gcc/gimple-lower-bitint.cc 2023-11-23 16:17:59.217298778 +0100
> @@ -5635,6 +5635,21 @@ gimple_lower_bitint (void)
> break;
> }
> }
> + /* Similarly, e.g. with -frounding-math casts from _BitInt INTEGER_CSTs
> + to floating point types need to be rewritten. */
> + else if (SCALAR_FLOAT_TYPE_P (type))
> + {
> + gimple *g = SSA_NAME_DEF_STMT (s);
> + if (is_gimple_assign (g) && gimple_assign_rhs_code (g) == FLOAT_EXPR)
> + {
I think this would combine with the virtual operand code up to the
is_gimle_assign () check.
But I also wonder if when you disable enough passes you could
for example see switch (bit-int-cst) or if (bit-int-cst ...)
as well? Given we have PROP_gimple_lbitint couldn't we set that
optimistically say during gimplification when we didn't see any
bit-int, making sure to clear the property during inlining when
the inlined function doesn't have it set? Or maybe require
frontends to opt-in into features that require additional
processing, indicating that with a bit in struct function
(or a flag on the decl or via a langhook)? There's other
passes that we could gate (coroutine stuff, omp stuff?)
Otherwise the patch looks OK, I'm just questioning the way we
try to avoid running it.
Thanks,
Richard.
> + tree t = gimple_assign_rhs1 (g);
> + if (TREE_CODE (t) == INTEGER_CST
> + && TREE_CODE (TREE_TYPE (t)) == BITINT_TYPE
> + && (bitint_precision_kind (TREE_TYPE (t))
> + != bitint_prec_small))
> + break;
> + }
> + }
> }
> if (i == num_ssa_names)
> return 0;
> @@ -5855,6 +5870,21 @@ gimple_lower_bitint (void)
> has_large_huge = true;
> }
> }
> + /* Similarly, e.g. with -frounding-math casts from _BitInt INTEGER_CSTs
> + to floating point types need to be rewritten. */
> + else if (SCALAR_FLOAT_TYPE_P (type))
> + {
> + gimple *g = SSA_NAME_DEF_STMT (s);
> + if (is_gimple_assign (g) && gimple_assign_rhs_code (g) == FLOAT_EXPR)
> + {
> + tree t = gimple_assign_rhs1 (g);
> + if (TREE_CODE (t) == INTEGER_CST
> + && TREE_CODE (TREE_TYPE (t)) == BITINT_TYPE
> + && (bitint_precision_kind (TREE_TYPE (t))
> + >= bitint_prec_large))
> + has_large_huge = true;
> + }
> + }
> }
> for (i = first_large_huge; i < num_ssa_names; ++i)
> {
> @@ -6182,6 +6212,19 @@ gimple_lower_bitint (void)
> {
> bitint_prec_kind this_kind
> = bitint_precision_kind (TREE_TYPE (t));
> + if (this_kind > kind)
> + kind = this_kind;
> + }
> + }
> + if (is_gimple_assign (stmt)
> + && gimple_assign_rhs_code (stmt) == FLOAT_EXPR)
> + {
> + t = gimple_assign_rhs1 (stmt);
> + if (TREE_CODE (TREE_TYPE (t)) == BITINT_TYPE
> + && TREE_CODE (t) == INTEGER_CST)
> + {
> + bitint_prec_kind this_kind
> + = bitint_precision_kind (TREE_TYPE (t));
> if (this_kind > kind)
> kind = this_kind;
> }
> --- gcc/testsuite/gcc.dg/bitint-42.c.jj 2023-11-23 16:50:52.392502318 +0100
> +++ gcc/testsuite/gcc.dg/bitint-42.c 2023-11-23 16:42:08.559881704 +0100
> @@ -0,0 +1,9 @@
> +/* PR middle-end/112679 */
> +/* { dg-do compile { target bitint } } */
> +/* { dg-options "-frounding-math" } */
> +
> +float
> +foo (void)
> +{
> + return 0x353eab28b46b03ea99b84f9736cd8dbe5e986915a0383c3cb381c0da41e31b3621c75fd53262bfcb1b0e6251dbf00f3988784e29b08b65640c263e4d0959832a20e2ff5245be1e60uwb;
> +}
>
> Jakub
>
>
--
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] 4+ messages in thread* Re: [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]
2023-11-24 9:20 ` Richard Biener
@ 2023-11-24 10:02 ` Jakub Jelinek
2023-11-24 10:08 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2023-11-24 10:02 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Fri, Nov 24, 2023 at 10:20:01AM +0100, Richard Biener wrote:
> > + /* Similarly, e.g. with -frounding-math casts from _BitInt INTEGER_CSTs
> > + to floating point types need to be rewritten. */
> > + else if (SCALAR_FLOAT_TYPE_P (type))
> > + {
> > + gimple *g = SSA_NAME_DEF_STMT (s);
> > + if (is_gimple_assign (g) && gimple_assign_rhs_code (g) == FLOAT_EXPR)
> > + {
>
> I think this would combine with the virtual operand code up to the
> is_gimle_assign () check.
Only the gimple *g = SSA_NAME_DEF_STMT (s); if (is_gimple_assign (g) &&
part is the same between the two, but virtual operands will not be seen
on FLOAT_EXPRs and stores won't be seen for the others, so I think it
is cheaper to handle them separately.
> But I also wonder if when you disable enough passes you could
> for example see switch (bit-int-cst) or if (bit-int-cst ...)
> as well? Given we have PROP_gimple_lbitint couldn't we set that
> optimistically say during gimplification when we didn't see any
> bit-int, making sure to clear the property during inlining when
> the inlined function doesn't have it set? Or maybe require
> frontends to opt-in into features that require additional
> processing, indicating that with a bit in struct function
> (or a flag on the decl or via a langhook)? There's other
> passes that we could gate (coroutine stuff, omp stuff?)
I've initially wanted to set a per-function flag somewhere, either
in the FEs or during gimplification, but it turned out that ensuring
the flag is set means I'd need to test it in a lot of places and slow
down processing of everything in there for the checks if any operand
is BITINT_TYPE.
Then I came up with the idea I can just check if any SSA_NAME has non-small
BITINT_TYPE; but of course the INTEGER_CST stores and now the FLOAT_EXPRs
with those ruin that a little bit.
What we could do perhaps cheaply at least for now is set some flag in
build_bitint_type (after all, we have one only, bitint_type_cache != NULL,
except it is static; just probably LTO streaming in bypasses that),
so perhaps we could return early or even gate the pass on no BITINT_TYPEs
created in the IL. That would help non-LTO with all non-C languages
including C++. And for C could be also effective, as long as system headers
don't start adding _BitInt types in there...
E.g. there was a risk in one of my attempts that stdbit.h would have one,
but in the end it won't.
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]
2023-11-24 10:02 ` Jakub Jelinek
@ 2023-11-24 10:08 ` Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2023-11-24 10:08 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Fri, 24 Nov 2023, Jakub Jelinek wrote:
> On Fri, Nov 24, 2023 at 10:20:01AM +0100, Richard Biener wrote:
> > > + /* Similarly, e.g. with -frounding-math casts from _BitInt INTEGER_CSTs
> > > + to floating point types need to be rewritten. */
> > > + else if (SCALAR_FLOAT_TYPE_P (type))
> > > + {
> > > + gimple *g = SSA_NAME_DEF_STMT (s);
> > > + if (is_gimple_assign (g) && gimple_assign_rhs_code (g) == FLOAT_EXPR)
> > > + {
> >
> > I think this would combine with the virtual operand code up to the
> > is_gimle_assign () check.
>
> Only the gimple *g = SSA_NAME_DEF_STMT (s); if (is_gimple_assign (g) &&
> part is the same between the two, but virtual operands will not be seen
> on FLOAT_EXPRs and stores won't be seen for the others, so I think it
> is cheaper to handle them separately.
>
> > But I also wonder if when you disable enough passes you could
> > for example see switch (bit-int-cst) or if (bit-int-cst ...)
> > as well? Given we have PROP_gimple_lbitint couldn't we set that
> > optimistically say during gimplification when we didn't see any
> > bit-int, making sure to clear the property during inlining when
> > the inlined function doesn't have it set? Or maybe require
> > frontends to opt-in into features that require additional
> > processing, indicating that with a bit in struct function
> > (or a flag on the decl or via a langhook)? There's other
> > passes that we could gate (coroutine stuff, omp stuff?)
>
> I've initially wanted to set a per-function flag somewhere, either
> in the FEs or during gimplification, but it turned out that ensuring
> the flag is set means I'd need to test it in a lot of places and slow
> down processing of everything in there for the checks if any operand
> is BITINT_TYPE.
Yeah, wondered about that ...
> Then I came up with the idea I can just check if any SSA_NAME has non-small
> BITINT_TYPE; but of course the INTEGER_CST stores and now the FLOAT_EXPRs
> with those ruin that a little bit.
>
> What we could do perhaps cheaply at least for now is set some flag in
> build_bitint_type (after all, we have one only, bitint_type_cache != NULL,
> except it is static; just probably LTO streaming in bypasses that),
> so perhaps we could return early or even gate the pass on no BITINT_TYPEs
> created in the IL. That would help non-LTO with all non-C languages
> including C++. And for C could be also effective, as long as system headers
> don't start adding _BitInt types in there...
> E.g. there was a risk in one of my attempts that stdbit.h would have one,
> but in the end it won't.
I guess we have that flag already, it's 'bitint_type_chache' itself?
But it's a bit coarse indeed.
Richard.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-24 10:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-24 8:15 [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679] Jakub Jelinek
2023-11-24 9:20 ` Richard Biener
2023-11-24 10:02 ` Jakub Jelinek
2023-11-24 10:08 ` Richard Biener
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).