[PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]

[PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]

[Jakub Jelinek]

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

Re: [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]

[Richard Biener]

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)

Re: [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]

[Jakub Jelinek]

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

Re: [PATCH] lower-bitint: Lower FLOAT_EXPR from BITINT_TYPE INTEGER_CST [PR112679]

[Richard Biener]

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.