diff mbox series

Fix PR90316

Message ID alpine.LSU.2.20.1905031324400.10704@zhemvz.fhfr.qr
State New
Headers show
Series Fix PR90316 | expand

Commit Message

Richard Biener May 3, 2019, 11:27 a.m. UTC 
I am testing the following patch to remove the code determining
the target virtual operand to walk to and instead compute it
based on the immediate dominator which we will reach anyways
(or a dominating block) during maybe_skip_until.

More simplifying might be possible but I'm trying to keep the
patch small and suitable for backporting up to the GCC 8 branch
where this regressed.

Note this will handle even more CFG shapes now and seems to
expose some uninit warnings in dwarf2out.c (at least).

Bootstrap & regtest running on x86_64-unknown-linux-gnu.

Richard.

2019-05-03  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/90316
	* tree-ssa-alias.c (maybe_skip_until): Pass in target BB,
	compute target on demand.
	(get_continuation_for_phi): Remove code walking stmts to
	get to a target virtual operand which could end up being
	quadratic.

Comments

Richard Biener May 3, 2019, 1:05 p.m. UTC | #1 
On Fri, 3 May 2019, Richard Biener wrote:

> 
> I am testing the following patch to remove the code determining
> the target virtual operand to walk to and instead compute it
> based on the immediate dominator which we will reach anyways
> (or a dominating block) during maybe_skip_until.
> 
> More simplifying might be possible but I'm trying to keep the
> patch small and suitable for backporting up to the GCC 8 branch
> where this regressed.
> 
> Note this will handle even more CFG shapes now and seems to
> expose some uninit warnings in dwarf2out.c (at least).

I can't seem to find an initializer that would "trap" on use
so I'm going to do

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c     (revision 270849)
+++ gcc/dwarf2out.c     (working copy)
@@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mod
   if (mode != GET_MODE (rtl) && GET_MODE (rtl) != VOIDmode)
     return NULL;
 
-  scalar_int_mode int_mode, inner_mode, op1_mode;
+  scalar_int_mode int_mode = SImode, inner_mode, op1_mode;
   switch (GET_CODE (rtl))
     {
     case POST_INC:

unless somebody comes up with something clever over the weekend...

Richard.

> Bootstrap & regtest running on x86_64-unknown-linux-gnu.
> 
> Richard.
> 
> 2019-05-03  Richard Biener  <rguenther@suse.de>
> 
> 	PR tree-optimization/90316
> 	* tree-ssa-alias.c (maybe_skip_until): Pass in target BB,
> 	compute target on demand.
> 	(get_continuation_for_phi): Remove code walking stmts to
> 	get to a target virtual operand which could end up being
> 	quadratic.
> 
> Index: gcc/tree-ssa-alias.c
> ===================================================================
> --- gcc/tree-ssa-alias.c	(revision 270847)
> +++ gcc/tree-ssa-alias.c	(working copy)
> @@ -2598,8 +2598,8 @@ stmt_kills_ref_p (gimple *stmt, tree ref
>     case false is returned.  The walk starts with VUSE, one argument of PHI.  */
>  
>  static bool
> -maybe_skip_until (gimple *phi, tree target, ao_ref *ref,
> -		  tree vuse, unsigned int *cnt, bitmap *visited,
> +maybe_skip_until (gimple *phi, tree &target, basic_block target_bb,
> +		  ao_ref *ref, tree vuse, unsigned int *cnt, bitmap *visited,
>  		  bool abort_on_visited,
>  		  void *(*translate)(ao_ref *, tree, void *, bool *),
>  		  void *data)
> @@ -2615,6 +2615,19 @@ maybe_skip_until (gimple *phi, tree targ
>    while (vuse != target)
>      {
>        gimple *def_stmt = SSA_NAME_DEF_STMT (vuse);
> +      /* If we are searching for the target VUSE by walking up to
> +         TARGET_BB dominating the original PHI we are finished once
> +	 we reach a default def or a definition in a block dominating
> +	 that block.  Update TARGET and return.  */
> +      if (!target
> +	  && (gimple_nop_p (def_stmt)
> +	      || dominated_by_p (CDI_DOMINATORS,
> +				 target_bb, gimple_bb (def_stmt))))
> +	{
> +	  target = vuse;
> +	  return true;
> +	}
> +
>        /* Recurse for PHI nodes.  */
>        if (gimple_code (def_stmt) == GIMPLE_PHI)
>  	{
> @@ -2698,49 +2711,17 @@ get_continuation_for_phi (gimple *phi, a
>        arg0 = NULL_TREE;
>      }
>    /* If not, look if we can reach such candidate by walking defs
> -     of a PHI arg without crossing other PHIs.  */
> -  if (! arg0)
> -    for (i = 0; i < nargs; ++i)
> -      {
> -	arg0 = PHI_ARG_DEF (phi, i);
> -	gimple *def = SSA_NAME_DEF_STMT (arg0);
> -	/* Backedges can't work.  */
> -	if (dominated_by_p (CDI_DOMINATORS,
> -			    gimple_bb (def), phi_bb))
> -	  continue;
> -	/* See below.  */
> -	if (gimple_code (def) == GIMPLE_PHI)
> -	  continue;
> -	while (! dominated_by_p (CDI_DOMINATORS,
> -				 phi_bb, gimple_bb (def)))
> -	  {
> -	    arg0 = gimple_vuse (def);
> -	    if (SSA_NAME_IS_DEFAULT_DEF (arg0))
> -	      break;
> -	    def = SSA_NAME_DEF_STMT (arg0);
> -	    if (gimple_code (def) == GIMPLE_PHI)
> -	      {
> -		/* Do not try to look through arbitrarily complicated
> -		   CFGs.  For those looking for the first VUSE starting
> -		   from the end of the immediate dominator of phi_bb
> -		   is likely faster.  */
> -		arg0 = NULL_TREE;
> -		goto next;
> -	      }
> -	  }
> -	break;
> -next:;
> -      }
> -  if (! arg0)
> -    return NULL_TREE;
> +     until we hit the immediate dominator.  maybe_skip_until will
> +     do that for us.  */
> +  basic_block dom = get_immediate_dominator (CDI_DOMINATORS, phi_bb);
>  
> -  /* Then check against the found candidate.  */
> +  /* Then check against the (to be) found candidate.  */
>    for (i = 0; i < nargs; ++i)
>      {
>        arg1 = PHI_ARG_DEF (phi, i);
>        if (arg1 == arg0)
>  	;
> -      else if (! maybe_skip_until (phi, arg0, ref, arg1, cnt, visited,
> +      else if (! maybe_skip_until (phi, arg0, dom, ref, arg1, cnt, visited,
>  				   abort_on_visited,
>  				   /* Do not translate when walking over
>  				      backedges.  */
>
Richard Sandiford May 4, 2019, 7:45 a.m. UTC | #2 
Richard Biener <rguenther@suse.de> writes:
> On Fri, 3 May 2019, Richard Biener wrote:
>
>> 
>> I am testing the following patch to remove the code determining
>> the target virtual operand to walk to and instead compute it
>> based on the immediate dominator which we will reach anyways
>> (or a dominating block) during maybe_skip_until.
>> 
>> More simplifying might be possible but I'm trying to keep the
>> patch small and suitable for backporting up to the GCC 8 branch
>> where this regressed.
>> 
>> Note this will handle even more CFG shapes now and seems to
>> expose some uninit warnings in dwarf2out.c (at least).
>
> I can't seem to find an initializer that would "trap" on use
> so I'm going to do
>
> Index: gcc/dwarf2out.c
> ===================================================================
> --- gcc/dwarf2out.c     (revision 270849)
> +++ gcc/dwarf2out.c     (working copy)
> @@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mod
>    if (mode != GET_MODE (rtl) && GET_MODE (rtl) != VOIDmode)
>      return NULL;
>  
> -  scalar_int_mode int_mode, inner_mode, op1_mode;
> +  scalar_int_mode int_mode = SImode, inner_mode, op1_mode;
>    switch (GET_CODE (rtl))
>      {
>      case POST_INC:
>
> unless somebody comes up with something clever over the weekend...

Nothing clever, but something rare like BImode is probably safer than
SImode, in case doing this masks real "uninitialised" uses in future.

Thanks,
Richard
Richard Biener May 6, 2019, 9:36 a.m. UTC | #3 
On Sat, 4 May 2019, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > On Fri, 3 May 2019, Richard Biener wrote:
> >
> >> 
> >> I am testing the following patch to remove the code determining
> >> the target virtual operand to walk to and instead compute it
> >> based on the immediate dominator which we will reach anyways
> >> (or a dominating block) during maybe_skip_until.
> >> 
> >> More simplifying might be possible but I'm trying to keep the
> >> patch small and suitable for backporting up to the GCC 8 branch
> >> where this regressed.
> >> 
> >> Note this will handle even more CFG shapes now and seems to
> >> expose some uninit warnings in dwarf2out.c (at least).
> >
> > I can't seem to find an initializer that would "trap" on use
> > so I'm going to do
> >
> > Index: gcc/dwarf2out.c
> > ===================================================================
> > --- gcc/dwarf2out.c     (revision 270849)
> > +++ gcc/dwarf2out.c     (working copy)
> > @@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mod
> >    if (mode != GET_MODE (rtl) && GET_MODE (rtl) != VOIDmode)
> >      return NULL;
> >  
> > -  scalar_int_mode int_mode, inner_mode, op1_mode;
> > +  scalar_int_mode int_mode = SImode, inner_mode, op1_mode;
> >    switch (GET_CODE (rtl))
> >      {
> >      case POST_INC:
> >
> > unless somebody comes up with something clever over the weekend...
> 
> Nothing clever, but something rare like BImode is probably safer than
> SImode, in case doing this masks real "uninitialised" uses in future.

Ick, and I forgot to install this hunk when I committed it this morning.

Thus fixed as obvious now, with BImode.

Richard.
Thomas Schwinge Oct. 30, 2019, 10:57 a.m. UTC | #4 
Hi!

On 2019-05-06T11:36:22+0200, Richard Biener <rguenther@suse.de> wrote:
> On Sat, 4 May 2019, Richard Sandiford wrote:
>> Richard Biener <rguenther@suse.de> writes:
>> > On Fri, 3 May 2019, Richard Biener wrote:
>> >> I am testing the following patch [...]

... which apparently also got backported to gcc-9-branch eventually...

>> >> Note this will handle even more CFG shapes now and seems to
>> >> expose some uninit warnings in dwarf2out.c (at least).

..., and when building gcc-9-branch with
'--enable-checking=yes,extra,rtl' (apparently I'm the only one doing
that, huh?), runs into the following (at least I suppose that's what's
meant with "expose some uninit warnings in dwarf2out.c"?):

    In file included from [...]/source-gcc/gcc/coretypes.h:433,
                     from [...]/source-gcc/gcc/dwarf2out.c:60:
    [...]/source-gcc/gcc/machmode.h: In function 'dw_loc_descr_node* mem_loc_descriptor(rtx, machine_mode, machine_mode, var_init_status)':
    [...]/source-gcc/gcc/machmode.h:520:42: error: 'int_mode' may be used uninitialized in this function [-Werror=maybe-uninitialized]
      520 |    ? mode_size_inline (mode) : mode_size[mode]);
          |                                          ^~~~
    [...]/source-gcc/gcc/dwarf2out.c:15464:19: note: 'int_mode' was declared here
    15464 |   scalar_int_mode int_mode, inner_mode, op1_mode;
          |                   ^~~~~~~~
    cc1plus: all warnings being treated as errors
    make[3]: *** [dwarf2out.o] Error 1

>> > I can't seem to find an initializer that would "trap" on use
>> > so I'm going to do
>> >
>> > Index: gcc/dwarf2out.c
>> > ===================================================================
>> > --- gcc/dwarf2out.c     (revision 270849)
>> > +++ gcc/dwarf2out.c     (working copy)
>> > @@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mod
>> >    if (mode != GET_MODE (rtl) && GET_MODE (rtl) != VOIDmode)
>> >      return NULL;
>> >  
>> > -  scalar_int_mode int_mode, inner_mode, op1_mode;
>> > +  scalar_int_mode int_mode = SImode, inner_mode, op1_mode;
>> >    switch (GET_CODE (rtl))
>> >      {
>> >      case POST_INC:
>> >
>> > unless somebody comes up with something clever over the weekend...
>> 
>> Nothing clever, but something rare like BImode is probably safer than
>> SImode, in case doing this masks real "uninitialised" uses in future.
>
> Ick, and I forgot to install this hunk when I committed it this morning.
>
> Thus fixed as obvious now, with BImode.

..., so I backported that fix (or is it rather "fix"?) to gcc-9-branch in
r277608 "Avoid '-Wmaybe-uninitialized' diagnostic in 'gcc/dwarf2out.c'",
see attached.


Grüße
 Thomas
Jakub Jelinek Oct. 30, 2019, 11:19 a.m. UTC | #5 
On Wed, Oct 30, 2019 at 11:57:12AM +0100, Thomas Schwinge wrote:
> ..., and when building gcc-9-branch with
> '--enable-checking=yes,extra,rtl' (apparently I'm the only one doing
> that, huh?), runs into the following (at least I suppose that's what's

I'm testing release branches with
../configure --enable-languages=default,ada,obj-c++,lto,go,brig,d --enable-checking=yes,rtl,extra
too, and saw (last time I've bootstrapped/regtested gcc-9-branch
this way was Oct 21st though):
../../gcc/machmode.h: In function ‘dw_loc_descr_node* mem_loc_descriptor(rtx, machine_mode, machine_mode, var_init_status)’:
../../gcc/machmode.h:520:42: warning: ‘int_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  520 |    ? mode_size_inline (mode) : mode_size[mode]);
      |                                          ^~~~
../../gcc/dwarf2out.c:15464:19: note: ‘int_mode’ was declared here
15464 |   scalar_int_mode int_mode, inner_mode, op1_mode;
      |                   ^~~~~~~~
and no fatal error.  Are you using --enable-werror or something similar
in addition?

	Jakub
Thomas Schwinge Oct. 30, 2019, 11:28 a.m. UTC | #6 
Hi!

On 2019-10-30T12:19:28+0100, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Oct 30, 2019 at 11:57:12AM +0100, Thomas Schwinge wrote:
>> ..., and when building gcc-9-branch with
>> '--enable-checking=yes,extra,rtl' (apparently I'm the only one doing
>> that, huh?), runs into the following (at least I suppose that's what's
>
> I'm testing release branches with
> ../configure --enable-languages=default,ada,obj-c++,lto,go,brig,d --enable-checking=yes,rtl,extra
> too, and saw (last time I've bootstrapped/regtested gcc-9-branch
> this way was Oct 21st though):
> ../../gcc/machmode.h: In function ‘dw_loc_descr_node* mem_loc_descriptor(rtx, machine_mode, machine_mode, var_init_status)’:
> ../../gcc/machmode.h:520:42: warning: ‘int_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   520 |    ? mode_size_inline (mode) : mode_size[mode]);
>       |                                          ^~~~
> ../../gcc/dwarf2out.c:15464:19: note: ‘int_mode’ was declared here
> 15464 |   scalar_int_mode int_mode, inner_mode, op1_mode;
>       |                   ^~~~~~~~
> and no fatal error.  Are you using --enable-werror or something similar
> in addition?

Eh, you're right, of course: '--enable-werror', not '--enable-checking'
is relevant here.  (Recovering from a cold -- brain still a bit "slow".)


Grüße
 Thomas
diff mbox series

Patch

Index: gcc/tree-ssa-alias.c
===================================================================
--- gcc/tree-ssa-alias.c	(revision 270847)
+++ gcc/tree-ssa-alias.c	(working copy)
@@ -2598,8 +2598,8 @@  stmt_kills_ref_p (gimple *stmt, tree ref
    case false is returned.  The walk starts with VUSE, one argument of PHI.  */
 
 static bool
-maybe_skip_until (gimple *phi, tree target, ao_ref *ref,
-		  tree vuse, unsigned int *cnt, bitmap *visited,
+maybe_skip_until (gimple *phi, tree &target, basic_block target_bb,
+		  ao_ref *ref, tree vuse, unsigned int *cnt, bitmap *visited,
 		  bool abort_on_visited,
 		  void *(*translate)(ao_ref *, tree, void *, bool *),
 		  void *data)
@@ -2615,6 +2615,19 @@  maybe_skip_until (gimple *phi, tree targ
   while (vuse != target)
     {
       gimple *def_stmt = SSA_NAME_DEF_STMT (vuse);
+      /* If we are searching for the target VUSE by walking up to
+         TARGET_BB dominating the original PHI we are finished once
+	 we reach a default def or a definition in a block dominating
+	 that block.  Update TARGET and return.  */
+      if (!target
+	  && (gimple_nop_p (def_stmt)
+	      || dominated_by_p (CDI_DOMINATORS,
+				 target_bb, gimple_bb (def_stmt))))
+	{
+	  target = vuse;
+	  return true;
+	}
+
       /* Recurse for PHI nodes.  */
       if (gimple_code (def_stmt) == GIMPLE_PHI)
 	{
@@ -2698,49 +2711,17 @@  get_continuation_for_phi (gimple *phi, a
       arg0 = NULL_TREE;
     }
   /* If not, look if we can reach such candidate by walking defs
-     of a PHI arg without crossing other PHIs.  */
-  if (! arg0)
-    for (i = 0; i < nargs; ++i)
-      {
-	arg0 = PHI_ARG_DEF (phi, i);
-	gimple *def = SSA_NAME_DEF_STMT (arg0);
-	/* Backedges can't work.  */
-	if (dominated_by_p (CDI_DOMINATORS,
-			    gimple_bb (def), phi_bb))
-	  continue;
-	/* See below.  */
-	if (gimple_code (def) == GIMPLE_PHI)
-	  continue;
-	while (! dominated_by_p (CDI_DOMINATORS,
-				 phi_bb, gimple_bb (def)))
-	  {
-	    arg0 = gimple_vuse (def);
-	    if (SSA_NAME_IS_DEFAULT_DEF (arg0))
-	      break;
-	    def = SSA_NAME_DEF_STMT (arg0);
-	    if (gimple_code (def) == GIMPLE_PHI)
-	      {
-		/* Do not try to look through arbitrarily complicated
-		   CFGs.  For those looking for the first VUSE starting
-		   from the end of the immediate dominator of phi_bb
-		   is likely faster.  */
-		arg0 = NULL_TREE;
-		goto next;
-	      }
-	  }
-	break;
-next:;
-      }
-  if (! arg0)
-    return NULL_TREE;
+     until we hit the immediate dominator.  maybe_skip_until will
+     do that for us.  */
+  basic_block dom = get_immediate_dominator (CDI_DOMINATORS, phi_bb);
 
-  /* Then check against the found candidate.  */
+  /* Then check against the (to be) found candidate.  */
   for (i = 0; i < nargs; ++i)
     {
       arg1 = PHI_ARG_DEF (phi, i);
       if (arg1 == arg0)
 	;
-      else if (! maybe_skip_until (phi, arg0, ref, arg1, cnt, visited,
+      else if (! maybe_skip_until (phi, arg0, dom, ref, arg1, cnt, visited,
 				   abort_on_visited,
 				   /* Do not translate when walking over
 				      backedges.  */