diff mbox series

[3/12] phiopt: Fix phiopt ICE on vops [PR102989]

Message ID ZNPYJTDaCXXiAkUh@tucnak
State Unresolved
Headers
Series GCC _BitInt support [PR102989] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Aug. 9, 2023, 6:17 p.m. UTC 
  Hi!

I've ran into ICE on gcc.dg/torture/bitint-42.c with -O1 or -Os
when enabling expensive tests, and unfortunately I can't reproduce without
_BitInt.  The IL before phiopt3 has:
  <bb 87> [local count: 203190070]:
  # .MEM_428 = VDEF <.MEM_367>
  bitint.159 = VIEW_CONVERT_EXPR<unsigned long[8]>(*.LC3);
  goto <bb 89>; [100.00%]

  <bb 88> [local count: 203190070]:
  # .MEM_427 = VDEF <.MEM_367>
  bitint.159 = VIEW_CONVERT_EXPR<unsigned long[8]>(*.LC4);

  <bb 89> [local count: 406380139]:
  # .MEM_368 = PHI <.MEM_428(87), .MEM_427(88)>
  # VUSE <.MEM_368>
  _123 = VIEW_CONVERT_EXPR<unsigned long[8]>(r495[i_107].D.2780)[0];
and factor_out_conditional_operation is called on the vop PHI, it
sees it has exactly two operands and defining statements of both
PHI arguments are converts (VCEs in this case), so it thinks it is
a good idea to try to optimize that and while doing that it constructs
void type SSA_NAMEs and the like.

2023-08-09  <jakub@redhat.com>

	PR c/102989
	* tree-ssa-phiopt.cc (factor_out_conditional_operation): Punt for
	vops.


	Jakub

Comments

Andrew Pinski Aug. 9, 2023, 6:27 p.m. UTC | #1 
On Wed, Aug 9, 2023 at 11:17 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi!
>
> I've ran into ICE on gcc.dg/torture/bitint-42.c with -O1 or -Os
> when enabling expensive tests, and unfortunately I can't reproduce without
> _BitInt.  The IL before phiopt3 has:
>   <bb 87> [local count: 203190070]:
>   # .MEM_428 = VDEF <.MEM_367>
>   bitint.159 = VIEW_CONVERT_EXPR<unsigned long[8]>(*.LC3);
>   goto <bb 89>; [100.00%]
>
>   <bb 88> [local count: 203190070]:
>   # .MEM_427 = VDEF <.MEM_367>
>   bitint.159 = VIEW_CONVERT_EXPR<unsigned long[8]>(*.LC4);
>
>   <bb 89> [local count: 406380139]:
>   # .MEM_368 = PHI <.MEM_428(87), .MEM_427(88)>
>   # VUSE <.MEM_368>
>   _123 = VIEW_CONVERT_EXPR<unsigned long[8]>(r495[i_107].D.2780)[0];
> and factor_out_conditional_operation is called on the vop PHI, it
> sees it has exactly two operands and defining statements of both
> PHI arguments are converts (VCEs in this case), so it thinks it is
> a good idea to try to optimize that and while doing that it constructs
> void type SSA_NAMEs and the like.

Maybe it is better to punt for VOPS after the call to
single_non_singleton_phi_for_edges since none of functions called
afterwards support VOPs.
That is something like:
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index ff36bb0119b..d0b659042a7 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -4165,6 +4165,10 @@ pass_phiopt::execute (function *)
       arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
       arg1 = gimple_phi_arg_def (phi, e2->dest_idx);

+      /* Can't do anything with a VOP here.  */
+      if (SSA_NAME_IS_VIRTUAL_OPERAND (arg0))
+       continue;
+
       /* Something is wrong if we cannot find the arguments in the PHI
          node.  */
       gcc_assert (arg0 != NULL_TREE && arg1 != NULL_TREE);

Thanks,
Andrew Pinski

>
> 2023-08-09  <jakub@redhat.com>
>
>         PR c/102989
>         * tree-ssa-phiopt.cc (factor_out_conditional_operation): Punt for
>         vops.
>
> --- gcc/tree-ssa-phiopt.cc.jj   2023-08-08 15:55:09.508122417 +0200
> +++ gcc/tree-ssa-phiopt.cc      2023-08-09 15:55:23.762314103 +0200
> @@ -241,6 +241,7 @@ factor_out_conditional_operation (edge e
>      }
>
>    if (TREE_CODE (arg0) != SSA_NAME
> +      || SSA_NAME_IS_VIRTUAL_OPERAND (arg0)
>        || (TREE_CODE (arg1) != SSA_NAME
>           && TREE_CODE (arg1) != INTEGER_CST))
>      return NULL;
>
>         Jakub
>
Jakub Jelinek Aug. 9, 2023, 8 p.m. UTC | #2 
On Wed, Aug 09, 2023 at 11:27:48AM -0700, Andrew Pinski wrote:
> Maybe it is better to punt for VOPS after the call to
> single_non_singleton_phi_for_edges since none of functions called
> afterwards support VOPs.
> That is something like:
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index ff36bb0119b..d0b659042a7 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -4165,6 +4165,10 @@ pass_phiopt::execute (function *)
>        arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
>        arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> 
> +      /* Can't do anything with a VOP here.  */
> +      if (SSA_NAME_IS_VIRTUAL_OPERAND (arg0))
> +       continue;
> +

That would ICE if arg0 isn't SSA_NAME (e.g. is INTEGER_CST).
I think more canonical test for virtual phis is
if (virtual_operand_p (gimple_phi_result (phi)))

Shall already single_non_singleton_phi_for_edges punt if there is
a virtual phi with different arguments from the edges (or if there
is a single virtual phi)?

	Jakub
Andrew Pinski Aug. 9, 2023, 8:06 p.m. UTC | #3 
On Wed, Aug 9, 2023 at 1:01 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Aug 09, 2023 at 11:27:48AM -0700, Andrew Pinski wrote:
> > Maybe it is better to punt for VOPS after the call to
> > single_non_singleton_phi_for_edges since none of functions called
> > afterwards support VOPs.
> > That is something like:
> > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > index ff36bb0119b..d0b659042a7 100644
> > --- a/gcc/tree-ssa-phiopt.cc
> > +++ b/gcc/tree-ssa-phiopt.cc
> > @@ -4165,6 +4165,10 @@ pass_phiopt::execute (function *)
> >        arg0 = gimple_phi_arg_def (phi, e1->dest_idx);
> >        arg1 = gimple_phi_arg_def (phi, e2->dest_idx);
> >
> > +      /* Can't do anything with a VOP here.  */
> > +      if (SSA_NAME_IS_VIRTUAL_OPERAND (arg0))
> > +       continue;
> > +
>
> That would ICE if arg0 isn't SSA_NAME (e.g. is INTEGER_CST).
> I think more canonical test for virtual phis is
> if (virtual_operand_p (gimple_phi_result (phi)))
>
> Shall already single_non_singleton_phi_for_edges punt if there is
> a virtual phi with different arguments from the edges (or if there
> is a single virtual phi)?

That was my next thought, returning NULL from
single_non_singleton_phi_for_edges if it would return a virtual OP
might be better even.
Either version of these patches are ok with me (though I am not the
maintainer here).

Thanks,
Andrew

>
>         Jakub
>
Jakub Jelinek Aug. 9, 2023, 8:18 p.m. UTC | #4 
On Wed, Aug 09, 2023 at 01:06:22PM -0700, Andrew Pinski wrote:
> That was my next thought, returning NULL from
> single_non_singleton_phi_for_edges if it would return a virtual OP
> might be better even.
> Either version of these patches are ok with me (though I am not the
> maintainer here).

In patch form that would be (but so far untested):

2023-08-09  <jakub@redhat.com>

	PR c/102989
	* tree-ssa-phiopt.cc (single_non_singleton_phi_for_edges): Never
	return virtual phis and return NULL if there is a virtual phi
	where the arguments from E0 and E1 edges aren't equal.

--- gcc/tree-ssa-phiopt.cc.jj	2023-08-09 22:08:07.974563266 +0200
+++ gcc/tree-ssa-phiopt.cc	2023-08-09 22:11:37.291517911 +0200
@@ -63,7 +63,13 @@ single_non_singleton_phi_for_edges (gimp
   gimple_stmt_iterator i;
   gphi *phi = NULL;
   if (gimple_seq_singleton_p (seq))
-    return as_a <gphi *> (gsi_stmt (gsi_start (seq)));
+    {
+      phi = as_a <gphi *> (gsi_stmt (gsi_start (seq)));
+      /* Never return virtual phis.  */
+      if (virtual_operand_p (gimple_phi_result (phi)))
+	return NULL;
+      return phi;
+    }
   for (i = gsi_start (seq); !gsi_end_p (i); gsi_next (&i))
     {
       gphi *p = as_a <gphi *> (gsi_stmt (i));
@@ -72,6 +78,10 @@ single_non_singleton_phi_for_edges (gimp
 				       gimple_phi_arg_def (p, e1->dest_idx)))
 	continue;
 
+      /* Punt on virtual phis with different arguments from the edges.  */
+      if (virtual_operand_p (gimple_phi_result (p)))
+	return NULL;
+
       /* If we already have a PHI that has the two edge arguments are
 	 different, then return it is not a singleton for these PHIs. */
       if (phi)


	Jakub
Richard Biener Aug. 10, 2023, 6:52 a.m. UTC | #5 
On Wed, 9 Aug 2023, Jakub Jelinek wrote:

> On Wed, Aug 09, 2023 at 01:06:22PM -0700, Andrew Pinski wrote:
> > That was my next thought, returning NULL from
> > single_non_singleton_phi_for_edges if it would return a virtual OP
> > might be better even.
> > Either version of these patches are ok with me (though I am not the
> > maintainer here).
> 
> In patch form that would be (but so far untested):

LGTM

> 2023-08-09  <jakub@redhat.com>
> 
> 	PR c/102989
> 	* tree-ssa-phiopt.cc (single_non_singleton_phi_for_edges): Never
> 	return virtual phis and return NULL if there is a virtual phi
> 	where the arguments from E0 and E1 edges aren't equal.
> 
> --- gcc/tree-ssa-phiopt.cc.jj	2023-08-09 22:08:07.974563266 +0200
> +++ gcc/tree-ssa-phiopt.cc	2023-08-09 22:11:37.291517911 +0200
> @@ -63,7 +63,13 @@ single_non_singleton_phi_for_edges (gimp
>    gimple_stmt_iterator i;
>    gphi *phi = NULL;
>    if (gimple_seq_singleton_p (seq))
> -    return as_a <gphi *> (gsi_stmt (gsi_start (seq)));
> +    {
> +      phi = as_a <gphi *> (gsi_stmt (gsi_start (seq)));
> +      /* Never return virtual phis.  */
> +      if (virtual_operand_p (gimple_phi_result (phi)))
> +	return NULL;
> +      return phi;
> +    }
>    for (i = gsi_start (seq); !gsi_end_p (i); gsi_next (&i))
>      {
>        gphi *p = as_a <gphi *> (gsi_stmt (i));
> @@ -72,6 +78,10 @@ single_non_singleton_phi_for_edges (gimp
>  				       gimple_phi_arg_def (p, e1->dest_idx)))
>  	continue;
>  
> +      /* Punt on virtual phis with different arguments from the edges.  */
> +      if (virtual_operand_p (gimple_phi_result (p)))
> +	return NULL;
> +
>        /* If we already have a PHI that has the two edge arguments are
>  	 different, then return it is not a singleton for these PHIs. */
>        if (phi)
> 
> 
> 	Jakub
> 
>
diff mbox series

Patch

--- gcc/tree-ssa-phiopt.cc.jj	2023-08-08 15:55:09.508122417 +0200
+++ gcc/tree-ssa-phiopt.cc	2023-08-09 15:55:23.762314103 +0200
@@ -241,6 +241,7 @@  factor_out_conditional_operation (edge e
     }
 
   if (TREE_CODE (arg0) != SSA_NAME
+      || SSA_NAME_IS_VIRTUAL_OPERAND (arg0)
       || (TREE_CODE (arg1) != SSA_NAME
 	  && TREE_CODE (arg1) != INTEGER_CST))
     return NULL;