[PATCH] Fix PR90316

[PATCH] Fix PR90316

[Richard Biener]

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.

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.  */

Re: [PATCH] Fix PR90316

[Richard Biener]

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

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 Biener <rguenther@suse.de>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG NÌrnberg)

Re: [PATCH] Fix PR90316

[Richard Sandiford]

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

Re: [PATCH] Fix PR90316

[Richard Biener]

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.

Re: [PATCH] Fix PR90316

[Thomas Schwinge]

[-- Attachment #1.1: Type: text/plain, Size: 2735 bytes --]

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



[-- Attachment #1.2: 0001-Avoid-Wmaybe-uninitialized-diagnostic-i.gcc-9-branch.patch --]
[-- Type: text/x-diff, Size: 2361 bytes --]

From 3d82e409c5ef4d39908bd9798d096862af3eb4ca Mon Sep 17 00:00:00 2001
From: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Wed, 30 Oct 2019 10:50:58 +0000
Subject: [PATCH] Avoid '-Wmaybe-uninitialized' diagnostic in 'gcc/dwarf2out.c'

With '--enable-checking=yes,extra,rtl':

    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

Backport trunk r270903.

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

	* dwarf2out.c (mem_loc_descriptor): Initialize int_mode.

git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gcc-9-branch@277608 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog   | 8 ++++++++
 gcc/dwarf2out.c | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 910887a8451..ce4630ba078 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@
+2019-10-30  Thomas Schwinge  <thomas@codesourcery.com>
+
+	Backport from trunk:
+
+	2019-05-06  Richard Biener  <rguenther@suse.de>
+
+	* dwarf2out.c (mem_loc_descriptor): Initialize int_mode.
+
 2019-10-28  Uroš Bizjak  <ubizjak@gmail.com>
 
 	PR target/92225
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 55a73e12cc2..ae47387b763 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -15461,7 +15461,7 @@ mem_loc_descriptor (rtx rtl, machine_mode mode,
   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 = BImode, inner_mode, op1_mode;
   switch (GET_CODE (rtl))
     {
     case POST_INC:
-- 
2.17.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]

Re: [PATCH] Fix PR90316

[Jakub Jelinek]

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

Re: [PATCH] Fix PR90316

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]