[PATCH] tree-vect-generic: Fix up ICE with SSA_NAME_OCCURS_IN_ABNORMAL_PHI [PR109392]

[PATCH] tree-vect-generic: Fix up ICE with SSA_NAME_OCCURS_IN_ABNORMAL_PHI [PR109392]

[Jakub Jelinek]

Hi!

tree_vect_extract uses gimple-fold/match.pd to attempt to simplify
the BIT_FIELD_REF immediately.
Unfortunately, maybe_push_res_to_seq has:
  /* Play safe and do not allow abnormals to be mentioned in
     newly created statements.  */
  for (unsigned int i = 0; i < num_ops; ++i)
    if (TREE_CODE (ops[i]) == SSA_NAME
        && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (ops[i]))
      return NULL_TREE;

  if (num_ops > 0 && COMPARISON_CLASS_P (ops[0]))
    for (unsigned int i = 0; i < 2; ++i)
      if (TREE_CODE (TREE_OPERAND (ops[0], i)) == SSA_NAME
          && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (TREE_OPERAND (ops[0], i)))
        return NULL_TREE;
and so can fail in presence of SSA_NAME_OCCURS_IN_ABNORMAL_PHI SSA_NAMEs,
where we then trigger the assert that maybe_push_res_to_seq doesn't return
NULL.  The above is perfectly reasonable when trying to actually simplify
something that has been already created and let us just punt, but in the
tree_vect_extract case we have to build something always.

The following patch fixes it by building the BIT_FIELD_REF manually in
that case if the build+simplification failed.

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

2023-04-05  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/109392
	* tree-vect-generic.cc (tree_vec_extract): If maybe_push_res_to_seq
	fails, build BIT_FIELD_REF insn without trying to simplify it.

	* gcc.dg/pr109392.c: New test.

--- gcc/tree-vect-generic.cc.jj	2023-03-23 10:02:18.997935620 +0100
+++ gcc/tree-vect-generic.cc	2023-04-04 14:28:32.977729134 +0200
@@ -174,7 +174,16 @@ tree_vec_extract (gimple_stmt_iterator *
   opr.resimplify (NULL, follow_all_ssa_edges);
   gimple_seq stmts = NULL;
   tree res = maybe_push_res_to_seq (&opr, &stmts);
-  gcc_assert (res);
+  if (!res)
+    {
+      /* This can happen if SSA_NAME_OCCURS_IN_ABNORMAL_PHI are
+	 used.  Build BIT_FIELD_REF manually otherwise.  */
+      t = build3 (BIT_FIELD_REF, type, t, bitsize, bitpos);
+      res = make_ssa_name (type);
+      gimple *g = gimple_build_assign (res, t);
+      gsi_insert_before (gsi, g, GSI_SAME_STMT);
+      return res;
+    }
   gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
   return res;
 }
--- gcc/testsuite/gcc.dg/pr109392.c.jj	2023-04-04 14:36:03.096109943 +0200
+++ gcc/testsuite/gcc.dg/pr109392.c	2023-04-04 14:35:39.784452751 +0200
@@ -0,0 +1,15 @@
+/* PR tree-optimization/109392 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wno-psabi" } */
+
+typedef short __attribute__ ((__vector_size__ (64))) V;
+V v, w;
+void bar (void) __attribute__((returns_twice));
+
+V
+foo (V a, V b)
+{
+  bar ();
+  b &= v < b;
+  return (V) { foo (b, w)[3], (V) {}[3] };
+}

	Jakub

Re: [PATCH] tree-vect-generic: Fix up ICE with SSA_NAME_OCCURS_IN_ABNORMAL_PHI [PR109392]

[Richard Biener]

> Am 05.04.2023 um 10:58 schrieb Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> Hi!
> 
> tree_vect_extract uses gimple-fold/match.pd to attempt to simplify
> the BIT_FIELD_REF immediately.
> Unfortunately, maybe_push_res_to_seq has:
>  /* Play safe and do not allow abnormals to be mentioned in
>     newly created statements.  */
>  for (unsigned int i = 0; i < num_ops; ++i)
>    if (TREE_CODE (ops[i]) == SSA_NAME
>        && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (ops[i]))
>      return NULL_TREE;
> 
>  if (num_ops > 0 && COMPARISON_CLASS_P (ops[0]))
>    for (unsigned int i = 0; i < 2; ++i)
>      if (TREE_CODE (TREE_OPERAND (ops[0], i)) == SSA_NAME
>          && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (TREE_OPERAND (ops[0], i)))
>        return NULL_TREE;
> and so can fail in presence of SSA_NAME_OCCURS_IN_ABNORMAL_PHI SSA_NAMEs,
> where we then trigger the assert that maybe_push_res_to_seq doesn't return
> NULL.  The above is perfectly reasonable when trying to actually simplify
> something that has been already created and let us just punt, but in the
> tree_vect_extract case we have to build something always.
> 
> The following patch fixes it by building the BIT_FIELD_REF manually in
> that case if the build+simplification failed.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok, but maybe there’s a gimple_build overload that meanwhile implements the desired semantics?  It would probably need to specify the valueization hook to follow SSA edges beyond the sequence we’re currently building.

Richard 

> 2023-04-05  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR tree-optimization/109392
>    * tree-vect-generic.cc (tree_vec_extract): If maybe_push_res_to_seq
>    fails, build BIT_FIELD_REF insn without trying to simplify it.
> 
>    * gcc.dg/pr109392.c: New test.
> 
> --- gcc/tree-vect-generic.cc.jj    2023-03-23 10:02:18.997935620 +0100
> +++ gcc/tree-vect-generic.cc    2023-04-04 14:28:32.977729134 +0200
> @@ -174,7 +174,16 @@ tree_vec_extract (gimple_stmt_iterator *
>   opr.resimplify (NULL, follow_all_ssa_edges);
>   gimple_seq stmts = NULL;
>   tree res = maybe_push_res_to_seq (&opr, &stmts);
> -  gcc_assert (res);
> +  if (!res)
> +    {
> +      /* This can happen if SSA_NAME_OCCURS_IN_ABNORMAL_PHI are
> +     used.  Build BIT_FIELD_REF manually otherwise.  */
> +      t = build3 (BIT_FIELD_REF, type, t, bitsize, bitpos);
> +      res = make_ssa_name (type);
> +      gimple *g = gimple_build_assign (res, t);
> +      gsi_insert_before (gsi, g, GSI_SAME_STMT);
> +      return res;
> +    }
>   gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
>   return res;
> }
> --- gcc/testsuite/gcc.dg/pr109392.c.jj    2023-04-04 14:36:03.096109943 +0200
> +++ gcc/testsuite/gcc.dg/pr109392.c    2023-04-04 14:35:39.784452751 +0200
> @@ -0,0 +1,15 @@
> +/* PR tree-optimization/109392 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wno-psabi" } */
> +
> +typedef short __attribute__ ((__vector_size__ (64))) V;
> +V v, w;
> +void bar (void) __attribute__((returns_twice));
> +
> +V
> +foo (V a, V b)
> +{
> +  bar ();
> +  b &= v < b;
> +  return (V) { foo (b, w)[3], (V) {}[3] };
> +}
> 
>    Jakub
> 

Re: [PATCH] tree-vect-generic: Fix up ICE with SSA_NAME_OCCURS_IN_ABNORMAL_PHI [PR109392]

[Jakub Jelinek]

On Wed, Apr 05, 2023 at 02:11:08PM +0200, Richard Biener wrote:
> Ok, but maybe there’s a gimple_build overload that meanwhile implements
> the desired semantics?  It would probably need to specify the valueization
> hook to follow SSA edges beyond the sequence we’re currently building.

Jeff has apparently committed the patch in the meantime.
For gimple_build, did you mean to use it just for this fallback case,
or instead of the initial resimplification as well?

> > 2023-04-05  Jakub Jelinek  <jakub@redhat.com>
> > 
> >    PR tree-optimization/109392
> >    * tree-vect-generic.cc (tree_vec_extract): If maybe_push_res_to_seq
> >    fails, build BIT_FIELD_REF insn without trying to simplify it.
> > 
> >    * gcc.dg/pr109392.c: New test.
> > 
> > --- gcc/tree-vect-generic.cc.jj    2023-03-23 10:02:18.997935620 +0100
> > +++ gcc/tree-vect-generic.cc    2023-04-04 14:28:32.977729134 +0200
> > @@ -174,7 +174,16 @@ tree_vec_extract (gimple_stmt_iterator *
> >   opr.resimplify (NULL, follow_all_ssa_edges);
> >   gimple_seq stmts = NULL;
> >   tree res = maybe_push_res_to_seq (&opr, &stmts);
> > -  gcc_assert (res);
> > +  if (!res)
> > +    {
> > +      /* This can happen if SSA_NAME_OCCURS_IN_ABNORMAL_PHI are
> > +     used.  Build BIT_FIELD_REF manually otherwise.  */
> > +      t = build3 (BIT_FIELD_REF, type, t, bitsize, bitpos);
> > +      res = make_ssa_name (type);
> > +      gimple *g = gimple_build_assign (res, t);
> > +      gsi_insert_before (gsi, g, GSI_SAME_STMT);
> > +      return res;
> > +    }
> >   gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
> >   return res;
> > }

	Jakub

Re: [PATCH] tree-vect-generic: Fix up ICE with SSA_NAME_OCCURS_IN_ABNORMAL_PHI [PR109392]

[Richard Biener]

On Tue, 11 Apr 2023, Jakub Jelinek wrote:

> On Wed, Apr 05, 2023 at 02:11:08PM +0200, Richard Biener wrote:
> > Ok, but maybe there?s a gimple_build overload that meanwhile implements
> > the desired semantics?  It would probably need to specify the valueization
> > hook to follow SSA edges beyond the sequence we?re currently building.
> 
> Jeff has apparently committed the patch in the meantime.
> For gimple_build, did you mean to use it just for this fallback case,
> or instead of the initial resimplification as well?

I was originally thinking of the initial resimplification.

But yes, using gimple_build would have simplified it to

  if (!res)
    res = gimple_build (&stmts, BIT_FIELD_REF, type, t, bitsize, bitpos);

With the gsi overloads the whole function should now be able to
be turned into

return gimple_build (gsi, true, GSI_SAME_STMT, UNKNOWN_LOCATION,
                     BIT_FIELD_REF, type, t, bitsize, bitpos);

I think.  Note that's not 100% equivalent to what we do now, so it's
probably better to defer to stage1.  What it doesn't do is

  /* We're using the resimplify API and maybe_push_res_to_seq to
     simplify the BIT_FIELD_REF but restrict the simplification to 
     a single stmt while at the same time following SSA edges for
     simplification with already emitted CTORs.  */

which is achieved by passing NULL as the gimple_seq * argument to
.resimplify.  The above replacement would instead do what a later
forwprop pass would do when folding.  ISTR I wanted to do minimal
changes when rewriting this.

Richard.

> > > 2023-04-05  Jakub Jelinek  <jakub@redhat.com>
> > > 
> > >    PR tree-optimization/109392
> > >    * tree-vect-generic.cc (tree_vec_extract): If maybe_push_res_to_seq
> > >    fails, build BIT_FIELD_REF insn without trying to simplify it.
> > > 
> > >    * gcc.dg/pr109392.c: New test.
> > > 
> > > --- gcc/tree-vect-generic.cc.jj    2023-03-23 10:02:18.997935620 +0100
> > > +++ gcc/tree-vect-generic.cc    2023-04-04 14:28:32.977729134 +0200
> > > @@ -174,7 +174,16 @@ tree_vec_extract (gimple_stmt_iterator *
> > >   opr.resimplify (NULL, follow_all_ssa_edges);
> > >   gimple_seq stmts = NULL;
> > >   tree res = maybe_push_res_to_seq (&opr, &stmts);
> > > -  gcc_assert (res);
> > > +  if (!res)
> > > +    {
> > > +      /* This can happen if SSA_NAME_OCCURS_IN_ABNORMAL_PHI are
> > > +     used.  Build BIT_FIELD_REF manually otherwise.  */
> > > +      t = build3 (BIT_FIELD_REF, type, t, bitsize, bitpos);
> > > +      res = make_ssa_name (type);
> > > +      gimple *g = gimple_build_assign (res, t);
> > > +      gsi_insert_before (gsi, g, GSI_SAME_STMT);
> > > +      return res;
> > > +    }
> > >   gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT);
> > >   return res;
> > > }
> 
> 	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)