[PATCH] lower-bitint: Handle INTEGER_CST rhs1 in handle_cast [PR113462]

[PATCH] lower-bitint: Handle INTEGER_CST rhs1 in handle_cast [PR113462]

[Jakub Jelinek]

Hi!

The following patch ICEs because fre3 leaves around unfolded
  _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(0);
statement and in handle_cast I was expecting just SSA_NAMEs for the
large/huge _BitInt to large/huge _BitInt casts; INTEGER_CST is something
we can handle in that case exactly the same, as the handle_operand recursion
handles those.

Of course, maybe we should also try to fold_stmt such cases somewhere in
bitint lowering preparation.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2024-01-20  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/113462
	* gimple-lower-bitint.cc (bitint_large_huge::handle_cast):
	Handle rhs1 INTEGER_CST like SSA_NAME.

	* gcc.dg/bitint-76.c: New test.

--- gcc/gimple-lower-bitint.cc.jj	2024-01-19 10:01:37.696673929 +0100
+++ gcc/gimple-lower-bitint.cc	2024-01-19 18:36:34.175254308 +0100
@@ -1250,7 +1250,7 @@ bitint_large_huge::handle_cast (tree lhs
 {
   tree rhs_type = TREE_TYPE (rhs1);
   gimple *g;
-  if (TREE_CODE (rhs1) == SSA_NAME
+  if ((TREE_CODE (rhs1) == SSA_NAME || TREE_CODE (rhs1) == INTEGER_CST)
       && TREE_CODE (lhs_type) == BITINT_TYPE
       && TREE_CODE (rhs_type) == BITINT_TYPE
       && bitint_precision_kind (lhs_type) >= bitint_prec_large
--- gcc/testsuite/gcc.dg/bitint-76.c.jj	2024-01-19 18:39:23.883980766 +0100
+++ gcc/testsuite/gcc.dg/bitint-76.c	2024-01-19 18:38:48.758451335 +0100
@@ -0,0 +1,16 @@
+/* PR tree-optimization/113462 */
+/* { dg-do compile { target bitint } } */
+/* { dg-options "-std=c23 -O2" } */
+
+#if __BITINT_MAXWIDTH__ >= 129
+typedef _BitInt(129) B;
+#else
+typedef _BitInt(63) B;
+#endif
+
+B
+foo (void)
+{
+  struct { B b; } s = {};
+  return s.b;
+}

	Jakub

Re: [PATCH] lower-bitint: Handle INTEGER_CST rhs1 in handle_cast [PR113462]

[Richard Biener]

> Am 20.01.2024 um 10:39 schrieb Jakub Jelinek <jakub@redhat.com>:
> 
> Hi!
> 
> The following patch ICEs because fre3 leaves around unfolded
>  _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(0);

Hmm, const_unop should handle this, I suppose either we fail to convert this to a NOP_EXPR or native encode/interpret do not handle it?

Richard 

> statement and in handle_cast I was expecting just SSA_NAMEs for the
> large/huge _BitInt to large/huge _BitInt casts; INTEGER_CST is something
> we can handle in that case exactly the same, as the handle_operand recursion
> handles those.
> 
> Of course, maybe we should also try to fold_stmt such cases somewhere in
> bitint lowering preparation.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok

Richard 

> 2024-01-20  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR tree-optimization/113462
>    * gimple-lower-bitint.cc (bitint_large_huge::handle_cast):
>    Handle rhs1 INTEGER_CST like SSA_NAME.
> 
>    * gcc.dg/bitint-76.c: New test.
> 
> --- gcc/gimple-lower-bitint.cc.jj    2024-01-19 10:01:37.696673929 +0100
> +++ gcc/gimple-lower-bitint.cc    2024-01-19 18:36:34.175254308 +0100
> @@ -1250,7 +1250,7 @@ bitint_large_huge::handle_cast (tree lhs
> {
>   tree rhs_type = TREE_TYPE (rhs1);
>   gimple *g;
> -  if (TREE_CODE (rhs1) == SSA_NAME
> +  if ((TREE_CODE (rhs1) == SSA_NAME || TREE_CODE (rhs1) == INTEGER_CST)
>       && TREE_CODE (lhs_type) == BITINT_TYPE
>       && TREE_CODE (rhs_type) == BITINT_TYPE
>       && bitint_precision_kind (lhs_type) >= bitint_prec_large
> --- gcc/testsuite/gcc.dg/bitint-76.c.jj    2024-01-19 18:39:23.883980766 +0100
> +++ gcc/testsuite/gcc.dg/bitint-76.c    2024-01-19 18:38:48.758451335 +0100
> @@ -0,0 +1,16 @@
> +/* PR tree-optimization/113462 */
> +/* { dg-do compile { target bitint } } */
> +/* { dg-options "-std=c23 -O2" } */
> +
> +#if __BITINT_MAXWIDTH__ >= 129
> +typedef _BitInt(129) B;
> +#else
> +typedef _BitInt(63) B;
> +#endif
> +
> +B
> +foo (void)
> +{
> +  struct { B b; } s = {};
> +  return s.b;
> +}
> 
>    Jakub
> 

Re: [PATCH] lower-bitint: Handle INTEGER_CST rhs1 in handle_cast [PR113462]

[Jakub Jelinek]

On Sat, Jan 20, 2024 at 12:11:10PM +0100, Richard Biener wrote:
> > The following patch ICEs because fre3 leaves around unfolded
> >  _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(0);
> 
> Hmm, const_unop should handle this, I suppose either we fail to convert this to a NOP_EXPR or native encode/interpret do not handle it?

We just propagate
  _BitInt(192) s;
  _BitInt(129) _1;

  s_6 = 0;
  _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(s_6);
to
  _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(0);
without trying to fold it in any way (note, fre1, I mistyped fre3).
And the earlier is result of sra, before that it is
  s = {};
  _1 = s.b;

	Jakub

Re: [PATCH] lower-bitint: Handle INTEGER_CST rhs1 in handle_cast [PR113462]

[Richard Biener]

On Sat, 20 Jan 2024, Jakub Jelinek wrote:

> On Sat, Jan 20, 2024 at 12:11:10PM +0100, Richard Biener wrote:
> > > The following patch ICEs because fre3 leaves around unfolded
> > >  _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(0);
> > 
> > Hmm, const_unop should handle this, I suppose either we fail to convert this to a NOP_EXPR or native encode/interpret do not handle it?
> 
> We just propagate
>   _BitInt(192) s;
>   _BitInt(129) _1;
> 
>   s_6 = 0;
>   _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(s_6);
> to
>   _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(0);
> without trying to fold it in any way (note, fre1, I mistyped fre3).

FRE folds all stmts it substitues into (:7182 and on):

  /* Fold the stmt if modified, this canonicalizes MEM_REFs we propagated
     into which is a requirement for the IPA devirt machinery.  */
  gimple *old_stmt = stmt;
  if (modified)
    {
      /* If a formerly non-invariant ADDR_EXPR is turned into an
         invariant one it was on a separate stmt.  */
      if (gimple_assign_single_p (stmt)
          && TREE_CODE (gimple_assign_rhs1 (stmt)) == ADDR_EXPR)
        recompute_tree_invariant_for_addr_expr (gimple_assign_rhs1 
(stmt));
      gimple_stmt_iterator prev = *gsi;
      gsi_prev (&prev);
      if (fold_stmt (gsi, follow_all_ssa_edges))
        {

so it's up to fold_stmt / match to optimize.  And match will also
try const_unop on unary ops with constant_for_folding op0 where
for VIEW_CONVERT_EXPR we should eventually try native_encode/interpret
via fold_view_convert_expr (but it uses a fixed 128 byte buffer which
might be too low here?).

Richard.

> And the earlier is result of sra, before that it is
>   s = {};
>   _1 = s.b;
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)

Re: [PATCH] lower-bitint: Handle INTEGER_CST rhs1 in handle_cast [PR113462]

[Richard Biener]

On Mon, 22 Jan 2024, Richard Biener wrote:

> On Sat, 20 Jan 2024, Jakub Jelinek wrote:
> 
> > On Sat, Jan 20, 2024 at 12:11:10PM +0100, Richard Biener wrote:
> > > > The following patch ICEs because fre3 leaves around unfolded
> > > >  _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(0);
> > > 
> > > Hmm, const_unop should handle this, I suppose either we fail to convert this to a NOP_EXPR or native encode/interpret do not handle it?
> > 
> > We just propagate
> >   _BitInt(192) s;
> >   _BitInt(129) _1;
> > 
> >   s_6 = 0;
> >   _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(s_6);
> > to
> >   _1 = VIEW_CONVERT_EXPR<_BitInt(129)>(0);
> > without trying to fold it in any way (note, fre1, I mistyped fre3).
> 
> FRE folds all stmts it substitues into (:7182 and on):
> 
>   /* Fold the stmt if modified, this canonicalizes MEM_REFs we propagated
>      into which is a requirement for the IPA devirt machinery.  */
>   gimple *old_stmt = stmt;
>   if (modified)
>     {
>       /* If a formerly non-invariant ADDR_EXPR is turned into an
>          invariant one it was on a separate stmt.  */
>       if (gimple_assign_single_p (stmt)
>           && TREE_CODE (gimple_assign_rhs1 (stmt)) == ADDR_EXPR)
>         recompute_tree_invariant_for_addr_expr (gimple_assign_rhs1 
> (stmt));
>       gimple_stmt_iterator prev = *gsi;
>       gsi_prev (&prev);
>       if (fold_stmt (gsi, follow_all_ssa_edges))
>         {
> 
> so it's up to fold_stmt / match to optimize.  And match will also
> try const_unop on unary ops with constant_for_folding op0 where
> for VIEW_CONVERT_EXPR we should eventually try native_encode/interpret
> via fold_view_convert_expr (but it uses a fixed 128 byte buffer which
> might be too low here?).

We run into

static tree
native_interpret_int (tree type, const unsigned char *ptr, int len)
{ 
...
  if (total_bytes > len
      || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
    return NULL_TREE;

OTOH using a V_C_E to "truncate" a _BitInt looks wrong?  OTOH the
check doesn't really handle native_encode_expr using the "proper"
wide_int encoding however that's exactly handled.  So it might be
a pre-existing issue that's only uncovered by large _BitInts
(__int128 might show similar issues?)

Richard.

Re: [PATCH] lower-bitint: Handle INTEGER_CST rhs1 in handle_cast [PR113462]

[Jakub Jelinek]

On Mon, Jan 22, 2024 at 11:27:52AM +0100, Richard Biener wrote:
> We run into
> 
> static tree
> native_interpret_int (tree type, const unsigned char *ptr, int len)
> { 
> ...
>   if (total_bytes > len
>       || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
>     return NULL_TREE;
> 
> OTOH using a V_C_E to "truncate" a _BitInt looks wrong?  OTOH the
> check doesn't really handle native_encode_expr using the "proper"
> wide_int encoding however that's exactly handled.  So it might be
> a pre-existing issue that's only uncovered by large _BitInts
> (__int128 might show similar issues?)

I guess the || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT
conditions make no sense, all we care is whether it fits in the buffer
or not.
But then there is
fold_view_convert_expr
(and other spots) which use
  /* We support up to 1024-bit values (for GCN/RISC-V V128QImode).  */
  unsigned char buffer[128];
or something similar.
Perhaps we could use XALLOCAVEC there instead (or use it only for the
larger sizes and keep the static buffer for the common case).

	Jakub

Re: [PATCH] lower-bitint: Handle INTEGER_CST rhs1 in handle_cast [PR113462]

[Richard Biener]

On Mon, 22 Jan 2024, Jakub Jelinek wrote:

> On Mon, Jan 22, 2024 at 11:27:52AM +0100, Richard Biener wrote:
> > We run into
> > 
> > static tree
> > native_interpret_int (tree type, const unsigned char *ptr, int len)
> > { 
> > ...
> >   if (total_bytes > len
> >       || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
> >     return NULL_TREE;
> > 
> > OTOH using a V_C_E to "truncate" a _BitInt looks wrong?  OTOH the
> > check doesn't really handle native_encode_expr using the "proper"
> > wide_int encoding however that's exactly handled.  So it might be
> > a pre-existing issue that's only uncovered by large _BitInts
> > (__int128 might show similar issues?)
> 
> I guess the || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT
> conditions make no sense, all we care is whether it fits in the buffer
> or not.
> But then there is
> fold_view_convert_expr
> (and other spots) which use
>   /* We support up to 1024-bit values (for GCN/RISC-V V128QImode).  */
>   unsigned char buffer[128];
> or something similar.
> Perhaps we could use XALLOCAVEC there instead (or use it only for the
> larger sizes and keep the static buffer for the common case).

Well, yes.  V_C_E folding could do this but then the native_encode_expr
API could also allow lazy allocation or so.

> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)

[PATCH] fold-const: Fold larger VIEW_CONVERT_EXPRs [PR113462]

[Jakub Jelinek]

Hi!

On Mon, Jan 22, 2024 at 11:56:11AM +0100, Richard Biener wrote:
> > I guess the || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT
> > conditions make no sense, all we care is whether it fits in the buffer
> > or not.
> > But then there is
> > fold_view_convert_expr
> > (and other spots) which use
> >   /* We support up to 1024-bit values (for GCN/RISC-V V128QImode).  */
> >   unsigned char buffer[128];
> > or something similar.
> > Perhaps we could use XALLOCAVEC there instead (or use it only for the
> > larger sizes and keep the static buffer for the common case).
> 
> Well, yes.  V_C_E folding could do this but then the native_encode_expr
> API could also allow lazy allocation or so.

native_encode_expr can't reallocate, it has to fill in whatever buffer it
has been called with, it can be in the middle of something else etc.

The following patch is what I meant, I think having some upper bound is
desirable so that we don't spend too much time trying to VCE fold 2GB
structures (after all, the APIs also use int for lengths) and similar and passed
make check-gcc check-g++ -j32 -k GCC_TEST_RUN_EXPENSIVE=1 RUNTESTFLAGS="GCC_TEST_RUN_EXPENSIVE=1 dg.exp='*bitint* pr112673.c builtin-stdc-bit-*.c pr112566-2.c pr112511.c' dg-torture.exp=*bitint* dfp.exp=*bitint*"
(my usual quick test for bitint related changes).  Ok for trunk if it passes
full bootstrap/regtest?

2024-01-22  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/113462
	* fold-const.cc (native_interpret_int): Don't punt if total_bytes
	is larger than HOST_BITS_PER_DOUBLE_INT / BITS_PER_UNIT.
	(fold_view_convert_expr): Use XALLOCAVEC buffers for types with
	sizes between 129 and 8192 bytes.

--- gcc/fold-const.cc.jj	2024-01-12 10:07:58.202851544 +0100
+++ gcc/fold-const.cc	2024-01-22 12:09:05.116253393 +0100
@@ -8773,8 +8773,7 @@ native_interpret_int (tree type, const u
   else
     total_bytes = GET_MODE_SIZE (SCALAR_INT_TYPE_MODE (type));
 
-  if (total_bytes > len
-      || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
+  if (total_bytes > len)
     return NULL_TREE;
 
   wide_int result = wi::from_buffer (ptr, total_bytes);
@@ -9329,9 +9328,10 @@ fold_view_convert_vector_encoding (tree
 static tree
 fold_view_convert_expr (tree type, tree expr)
 {
-  /* We support up to 1024-bit values (for GCN/RISC-V V128QImode).  */
   unsigned char buffer[128];
+  unsigned char *buf;
   int len;
+  HOST_WIDE_INT l;
 
   /* Check that the host and target are sane.  */
   if (CHAR_BIT != 8 || BITS_PER_UNIT != 8)
@@ -9341,11 +9341,23 @@ fold_view_convert_expr (tree type, tree
     if (tree res = fold_view_convert_vector_encoding (type, expr))
       return res;
 
-  len = native_encode_expr (expr, buffer, sizeof (buffer));
+  l = int_size_in_bytes (type);
+  if (l > (int) sizeof (buffer)
+      && l <= WIDE_INT_MAX_PRECISION / BITS_PER_UNIT)
+    {
+      buf = XALLOCAVEC (unsigned char, l);
+      len = l;
+    }
+  else
+    {
+      buf = buffer;
+      len = sizeof (buffer);
+    }
+  len = native_encode_expr (expr, buf, len);
   if (len == 0)
     return NULL_TREE;
 
-  return native_interpret_expr (type, buffer, len);
+  return native_interpret_expr (type, buf, len);
 }
 
 /* Build an expression for the address of T.  Folds away INDIRECT_REF


	Jakub

Re: [PATCH] fold-const: Fold larger VIEW_CONVERT_EXPRs [PR113462]

[Richard Biener]

On Mon, 22 Jan 2024, Jakub Jelinek wrote:

> Hi!
> 
> On Mon, Jan 22, 2024 at 11:56:11AM +0100, Richard Biener wrote:
> > > I guess the || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT
> > > conditions make no sense, all we care is whether it fits in the buffer
> > > or not.
> > > But then there is
> > > fold_view_convert_expr
> > > (and other spots) which use
> > >   /* We support up to 1024-bit values (for GCN/RISC-V V128QImode).  */
> > >   unsigned char buffer[128];
> > > or something similar.
> > > Perhaps we could use XALLOCAVEC there instead (or use it only for the
> > > larger sizes and keep the static buffer for the common case).
> > 
> > Well, yes.  V_C_E folding could do this but then the native_encode_expr
> > API could also allow lazy allocation or so.
> 
> native_encode_expr can't reallocate, it has to fill in whatever buffer it
> has been called with, it can be in the middle of something else etc.
> 
> The following patch is what I meant, I think having some upper bound is
> desirable so that we don't spend too much time trying to VCE fold 2GB
> structures (after all, the APIs also use int for lengths) and similar and passed
> make check-gcc check-g++ -j32 -k GCC_TEST_RUN_EXPENSIVE=1 RUNTESTFLAGS="GCC_TEST_RUN_EXPENSIVE=1 dg.exp='*bitint* pr112673.c builtin-stdc-bit-*.c pr112566-2.c pr112511.c' dg-torture.exp=*bitint* dfp.exp=*bitint*"
> (my usual quick test for bitint related changes).  Ok for trunk if it passes
> full bootstrap/regtest?

OK.

Thanks,
Richard.

> 2024-01-22  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/113462
> 	* fold-const.cc (native_interpret_int): Don't punt if total_bytes
> 	is larger than HOST_BITS_PER_DOUBLE_INT / BITS_PER_UNIT.
> 	(fold_view_convert_expr): Use XALLOCAVEC buffers for types with
> 	sizes between 129 and 8192 bytes.
> 
> --- gcc/fold-const.cc.jj	2024-01-12 10:07:58.202851544 +0100
> +++ gcc/fold-const.cc	2024-01-22 12:09:05.116253393 +0100
> @@ -8773,8 +8773,7 @@ native_interpret_int (tree type, const u
>    else
>      total_bytes = GET_MODE_SIZE (SCALAR_INT_TYPE_MODE (type));
>  
> -  if (total_bytes > len
> -      || total_bytes * BITS_PER_UNIT > HOST_BITS_PER_DOUBLE_INT)
> +  if (total_bytes > len)
>      return NULL_TREE;
>  
>    wide_int result = wi::from_buffer (ptr, total_bytes);
> @@ -9329,9 +9328,10 @@ fold_view_convert_vector_encoding (tree
>  static tree
>  fold_view_convert_expr (tree type, tree expr)
>  {
> -  /* We support up to 1024-bit values (for GCN/RISC-V V128QImode).  */
>    unsigned char buffer[128];
> +  unsigned char *buf;
>    int len;
> +  HOST_WIDE_INT l;
>  
>    /* Check that the host and target are sane.  */
>    if (CHAR_BIT != 8 || BITS_PER_UNIT != 8)
> @@ -9341,11 +9341,23 @@ fold_view_convert_expr (tree type, tree
>      if (tree res = fold_view_convert_vector_encoding (type, expr))
>        return res;
>  
> -  len = native_encode_expr (expr, buffer, sizeof (buffer));
> +  l = int_size_in_bytes (type);
> +  if (l > (int) sizeof (buffer)
> +      && l <= WIDE_INT_MAX_PRECISION / BITS_PER_UNIT)
> +    {
> +      buf = XALLOCAVEC (unsigned char, l);
> +      len = l;
> +    }
> +  else
> +    {
> +      buf = buffer;
> +      len = sizeof (buffer);
> +    }
> +  len = native_encode_expr (expr, buf, len);
>    if (len == 0)
>      return NULL_TREE;
>  
> -  return native_interpret_expr (type, buffer, len);
> +  return native_interpret_expr (type, buf, len);
>  }
>  
>  /* Build an expression for the address of T.  Folds away INDIRECT_REF
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)