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

[Richard Biener <rguenther@suse.de>]

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)