* [PATCH 3/12] phiopt: Fix phiopt ICE on vops [PR102989]
@ 2023-08-09 18:17 Jakub Jelinek
2023-08-09 18:27 ` Andrew Pinski
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2023-08-09 18:17 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
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.
--- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 3/12] phiopt: Fix phiopt ICE on vops [PR102989]
2023-08-09 18:17 [PATCH 3/12] phiopt: Fix phiopt ICE on vops [PR102989] Jakub Jelinek
@ 2023-08-09 18:27 ` Andrew Pinski
2023-08-09 20:00 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Pinski @ 2023-08-09 18:27 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, gcc-patches
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 3/12] phiopt: Fix phiopt ICE on vops [PR102989]
2023-08-09 18:27 ` Andrew Pinski
@ 2023-08-09 20:00 ` Jakub Jelinek
2023-08-09 20:06 ` Andrew Pinski
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2023-08-09 20:00 UTC (permalink / raw)
To: Andrew Pinski; +Cc: Richard Biener, gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/12] phiopt: Fix phiopt ICE on vops [PR102989]
2023-08-09 20:00 ` Jakub Jelinek
@ 2023-08-09 20:06 ` Andrew Pinski
2023-08-09 20:18 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Pinski @ 2023-08-09 20:06 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, gcc-patches
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/12] phiopt: Fix phiopt ICE on vops [PR102989]
2023-08-09 20:06 ` Andrew Pinski
@ 2023-08-09 20:18 ` Jakub Jelinek
2023-08-10 6:52 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2023-08-09 20:18 UTC (permalink / raw)
To: Richard Biener, Andrew Pinski; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 3/12] phiopt: Fix phiopt ICE on vops [PR102989]
2023-08-09 20:18 ` Jakub Jelinek
@ 2023-08-10 6:52 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2023-08-10 6:52 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Andrew Pinski, gcc-patches
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
>
>
--
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-10 6:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-09 18:17 [PATCH 3/12] phiopt: Fix phiopt ICE on vops [PR102989] Jakub Jelinek
2023-08-09 18:27 ` Andrew Pinski
2023-08-09 20:00 ` Jakub Jelinek
2023-08-09 20:06 ` Andrew Pinski
2023-08-09 20:18 ` Jakub Jelinek
2023-08-10 6:52 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).