[PATCH] Fix PR78515

[PATCH] Fix PR78515

[Richard Biener]

I am testing the following to beat some sanity into 
compute_complex_assign_jump_func.  There's still that odd 'stmt2'
hanging around that gets set to sth else than stmt with

  op1 = gimple_assign_rhs1 (stmt);

  if (TREE_CODE (op1) == SSA_NAME)
    {
      if (SSA_NAME_IS_DEFAULT_DEF (op1))
        index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1));
      else
        {
          index = load_from_param (fbi, info->descriptors,
                                   SSA_NAME_DEF_STMT (op1));
          stmt2 = SSA_NAME_DEF_STMT (op1);

I assume that the original code wanted to restrict its processing
to unary RHS of 'stmt' but still this "skips" arbitrary unary
operations in 'stmt'?  But maybe I'm not understanding jump functions
here.  If we have

  _2 = -param_1(D);
  _3 = ~_2;

and stmt is _3 then we create a unary pass through JF with - (and the ~
gets lost?).

Anyway, the following is a step in the right direction -- if you
want to address the above (maybe with some comments) you can
happily take the patch from here, otherwise we can followup this
patch.

Bootstrap & regtest running on x86_64-unknown-linux-gnu, ok for trunk?

Thanks,
Richard.

2016-11-25  Richard Biener  <rguenther@suse.de>

	PR ipa/78515
	* ipa-prop.c (compute_complex_assign_jump_func): Properly identify
	unary, binary and single RHSs.
	* tree.def (BIT_INSERT_EXPR): Adjust tree code name.

	* gcc.dg/torture/pr78515.c: New testcase.

diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 90c19fc..642111d 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -1177,29 +1177,37 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi,
 
   if (index >= 0)
     {
-      tree op2 = gimple_assign_rhs2 (stmt);
-
-      if (op2)
+      switch (gimple_assign_rhs_class (stmt))
 	{
-	  if (!is_gimple_ip_invariant (op2)
-	      || (TREE_CODE_CLASS (gimple_expr_code (stmt)) != tcc_comparison
-		  && !useless_type_conversion_p (TREE_TYPE (name),
-						 TREE_TYPE (op1))))
-	    return;
-
-	  ipa_set_jf_arith_pass_through (jfunc, index, op2,
-					 gimple_assign_rhs_code (stmt));
-	}
-      else if (gimple_assign_single_p (stmt))
-	{
-	  bool agg_p = parm_ref_data_pass_through_p (fbi, index, call, tc_ssa);
-	  ipa_set_jf_simple_pass_through (jfunc, index, agg_p);
+	case GIMPLE_BINARY_RHS:
+	  {
+	    tree op2 = gimple_assign_rhs2 (stmt);
+	    if (!is_gimple_ip_invariant (op2)
+		|| ((TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
+		     != tcc_comparison)
+		    && !useless_type_conversion_p (TREE_TYPE (name),
+						   TREE_TYPE (op1))))
+	      return;
+
+	    ipa_set_jf_arith_pass_through (jfunc, index, op2,
+					   gimple_assign_rhs_code (stmt));
+	    break;
+	  }
+	case GIMPLE_SINGLE_RHS:
+	  {
+	    bool agg_p = parm_ref_data_pass_through_p (fbi, index, call,
+						       tc_ssa);
+	    ipa_set_jf_simple_pass_through (jfunc, index, agg_p);
+	    break;
+	  }
+	case GIMPLE_UNARY_RHS:
+	  if (is_gimple_assign (stmt2)
+	      && gimple_assign_rhs_class (stmt2) == GIMPLE_UNARY_RHS
+	      && ! CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (stmt2)))
+	    ipa_set_jf_unary_pass_through (jfunc, index,
+					   gimple_assign_rhs_code (stmt2));
+	default:;
 	}
-      else if (is_gimple_assign (stmt2)
-	       && (gimple_expr_code (stmt2) != NOP_EXPR)
-	       && (TREE_CODE_CLASS (gimple_expr_code (stmt2)) == tcc_unary))
-	ipa_set_jf_unary_pass_through (jfunc, index,
-				       gimple_assign_rhs_code (stmt2));
       return;
     }
 
diff --git a/gcc/testsuite/gcc.dg/torture/pr78515.c b/gcc/testsuite/gcc.dg/torture/pr78515.c
new file mode 100644
index 0000000..d700db5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr78515.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-mavx512bw" { target x86_64-*-* i?86-*-* } } */
+
+typedef unsigned V __attribute__ ((vector_size (64)));
+
+V g;
+
+static V
+baz (V u, V v)
+{
+  g += u;
+  return v + g + 1;
+}
+
+static V
+bar (V u)
+{
+  u[0] = 0;
+  return baz(u, (V){});
+}
+
+V
+foo ()
+{
+  return (V){bar((V){})[0]};
+}
diff --git a/gcc/tree.def b/gcc/tree.def
index 2c35540..e093307 100644
--- a/gcc/tree.def
+++ b/gcc/tree.def
@@ -865,7 +865,7 @@ DEFTREECODE (FDESC_EXPR, "fdesc_expr", tcc_expression, 2)
    introducing a quaternary operation.
    The replaced bits shall be fully inside the container.  If the container
    is of vector type, then these bits shall be aligned with its elements.  */
-DEFTREECODE (BIT_INSERT_EXPR, "bit_field_insert", tcc_expression, 3)
+DEFTREECODE (BIT_INSERT_EXPR, "bit_insert_expr", tcc_expression, 3)
 
 /* Given two real or integer operands of the same type,
    returns a complex value of the corresponding complex type.  */

Re: [PATCH] Fix PR78515

[Martin Jambor]

Hi,

On Fri, Nov 25, 2016 at 12:01:38PM +0100, Richard Biener wrote:
> 
> I am testing the following to beat some sanity into 
> compute_complex_assign_jump_func.

That the function does not handle ternary operations (did we have them
since the beginning?) is clearly my fault and the patch is fine.
Please commit it.

> There's still that odd 'stmt2'
> hanging around that gets set to sth else than stmt with
> 
>   op1 = gimple_assign_rhs1 (stmt);
> 
>   if (TREE_CODE (op1) == SSA_NAME)
>     {
>       if (SSA_NAME_IS_DEFAULT_DEF (op1))
>         index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1));
>       else
>         {
>           index = load_from_param (fbi, info->descriptors,
>                                    SSA_NAME_DEF_STMT (op1));
>           stmt2 = SSA_NAME_DEF_STMT (op1);
> 
> I assume that the original code wanted to restrict its processing
> to unary RHS of 'stmt' but still this "skips" arbitrary unary
> operations in 'stmt'?  But maybe I'm not understanding jump functions
> here.  If we have
> 
>   _2 = -param_1(D);
>   _3 = ~_2;
> 
> and stmt is _3 then we create a unary pass through JF with - (and the ~
> gets lost?).
>

It definitely looks like that.  Unary arithmetic jump functions have
been added only recently with the IPA VRP propagation and I remember
staring at the stmt2 thingy for a while during review but then
apparently I forgot about it.

It seems to me that the check should refer to stmt, I will do that and
see what breaks and how the IL looks at that point and then decide
where to go from there.

Thanks,

Martin

Re: [PATCH] Fix PR78515

[Richard Biener]

On Fri, 25 Nov 2016, Martin Jambor wrote:

> Hi,
> 
> On Fri, Nov 25, 2016 at 12:01:38PM +0100, Richard Biener wrote:
> > 
> > I am testing the following to beat some sanity into 
> > compute_complex_assign_jump_func.
> 
> That the function does not handle ternary operations (did we have them
> since the beginning?) is clearly my fault and the patch is fine.

For quite some time indeed ;)

> Please commit it.

Will do after testing finished.

> > There's still that odd 'stmt2'
> > hanging around that gets set to sth else than stmt with
> > 
> >   op1 = gimple_assign_rhs1 (stmt);
> > 
> >   if (TREE_CODE (op1) == SSA_NAME)
> >     {
> >       if (SSA_NAME_IS_DEFAULT_DEF (op1))
> >         index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1));
> >       else
> >         {
> >           index = load_from_param (fbi, info->descriptors,
> >                                    SSA_NAME_DEF_STMT (op1));
> >           stmt2 = SSA_NAME_DEF_STMT (op1);
> > 
> > I assume that the original code wanted to restrict its processing
> > to unary RHS of 'stmt' but still this "skips" arbitrary unary
> > operations in 'stmt'?  But maybe I'm not understanding jump functions
> > here.  If we have
> > 
> >   _2 = -param_1(D);
> >   _3 = ~_2;
> > 
> > and stmt is _3 then we create a unary pass through JF with - (and the ~
> > gets lost?).
> >
> 
> It definitely looks like that.  Unary arithmetic jump functions have
> been added only recently with the IPA VRP propagation and I remember
> staring at the stmt2 thingy for a while during review but then
> apparently I forgot about it.
> 
> It seems to me that the check should refer to stmt, I will do that and
> see what breaks and how the IL looks at that point and then decide
> where to go from there.

it's the only use of stmt2 though, so it must have been added for some
reason... (maybe somebody wanted to handle simple copy-propagation?!).
I'd say rip it out and thus keep stmt2 == stmt.  There must be
a testcase added for this...

Richard.

Re: [PATCH] Fix PR78515

[Martin Sebor]

On 11/25/2016 06:55 AM, Richard Biener wrote:
> On Fri, 25 Nov 2016, Martin Jambor wrote:
>
>> Hi,
>>
>> On Fri, Nov 25, 2016 at 12:01:38PM +0100, Richard Biener wrote:
>>>
>>> I am testing the following to beat some sanity into
>>> compute_complex_assign_jump_func.
>>
>> That the function does not handle ternary operations (did we have them
>> since the beginning?) is clearly my fault and the patch is fine.
>
> For quite some time indeed ;)
>
>> Please commit it.
>
> Will do after testing finished.

The regression test is failing on powerpc64le due to the warnings
below:

FAIL: gcc.dg/torture/pr78515.c   -O0  (test for excess errors)
Excess errors:
/src/gcc/trunk/gcc/testsuite/gcc.dg/torture/pr78515.c:11:1: warning: GCC 
vector returned by reference: non-standard ABI extension with no 
compatibility guarantee [-Wpsabi]
/src/gcc/trunk/gcc/testsuite/gcc.dg/torture/pr78515.c:10:1: warning: GCC 
vector passed by reference: non-standard ABI extension with no 
compatibility guarantee [-Wpsabi]

Martin

Re: [PATCH] Fix PR78515

[Jakub Jelinek]

On Wed, Dec 14, 2016 at 05:10:23PM -0700, Martin Sebor wrote:
> The regression test is failing on powerpc64le due to the warnings
> below:
> 
> FAIL: gcc.dg/torture/pr78515.c   -O0  (test for excess errors)
> Excess errors:
> /src/gcc/trunk/gcc/testsuite/gcc.dg/torture/pr78515.c:11:1: warning: GCC
> vector returned by reference: non-standard ABI extension with no
> compatibility guarantee [-Wpsabi]
> /src/gcc/trunk/gcc/testsuite/gcc.dg/torture/pr78515.c:10:1: warning: GCC
> vector passed by reference: non-standard ABI extension with no compatibility
> guarantee [-Wpsabi]

David has fixed this recently, but just for AIX.  Generally, -Wno-psabi
is beneficial for all targets if it is needed on just one, I've committed
following:

2016-12-15  Jakub Jelinek  <jakub@redhat.com>

	* gcc.dg/tree-ssa/forwprop-35.c: Use -Wno-psabi everywhere.
	* gcc.dg/torture/pr78515.c: Likewise.
	* gcc.dg/pr69634.c: Likewise.

--- gcc/testsuite/gcc.dg/tree-ssa/forwprop-35.c.jj	2016-12-14 22:38:37.115389167 +0100
+++ gcc/testsuite/gcc.dg/tree-ssa/forwprop-35.c	2016-12-15 09:18:12.158020252 +0100
@@ -1,7 +1,6 @@
 /* { dg-do compile } */
-/* { dg-options "-O -fdump-tree-cddce1" } */
+/* { dg-options "-O -fdump-tree-cddce1 -Wno-psabi" } */
 /* { dg-additional-options "-msse2" { target { i?86-*-* x86_64-*-* } } } */
-/* { dg-additional-options "-Wno-psabi" { target { powerpc-ibm-aix* } } } */
 
 typedef int v4si __attribute__((vector_size(16)));
 typedef float v4sf __attribute__((vector_size(16)));
--- gcc/testsuite/gcc.dg/torture/pr78515.c.jj	2016-12-14 22:38:36.000000000 +0100
+++ gcc/testsuite/gcc.dg/torture/pr78515.c	2016-12-15 09:15:32.000142648 +0100
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
+/* { dg-additional-options "-Wno-psabi" } */
 /* { dg-additional-options "-mavx512bw" { target x86_64-*-* i?86-*-* } } */
-/* { dg-additional-options "-Wno-psabi" { target powerpc-ibm-aix* } } */
 
 typedef unsigned V __attribute__ ((vector_size (64)));
 
--- gcc/testsuite/gcc.dg/pr69634.c.jj	2016-12-14 22:38:37.251387392 +0100
+++ gcc/testsuite/gcc.dg/pr69634.c	2016-12-15 09:18:54.812455000 +0100
@@ -1,7 +1,6 @@
 /* { dg-do compile } */
-/* { dg-options "-O2 -fno-dce -fschedule-insns -fno-tree-vrp -fcompare-debug" } */
-/* { dg-additional-options "-Wno-psabi -mno-sse" { target i?86-*-* x86_64-*-* } } */
-/* { dg-additional-options "-Wno-psabi" { target powerpc-ibm-aix* } } */
+/* { dg-options "-O2 -fno-dce -fschedule-insns -fno-tree-vrp -fcompare-debug -Wno-psabi" } */
+/* { dg-additional-options "-mno-sse" { target i?86-*-* x86_64-*-* } } */
 /* { dg-require-effective-target scheduling } */
 
 typedef unsigned short u16;


	Jakub

Re: [PATCH] Fix PR78515

[David Edelsohn]

On Thu, Dec 15, 2016 at 3:23 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 14, 2016 at 05:10:23PM -0700, Martin Sebor wrote:
>> The regression test is failing on powerpc64le due to the warnings
>> below:
>>
>> FAIL: gcc.dg/torture/pr78515.c   -O0  (test for excess errors)
>> Excess errors:
>> /src/gcc/trunk/gcc/testsuite/gcc.dg/torture/pr78515.c:11:1: warning: GCC
>> vector returned by reference: non-standard ABI extension with no
>> compatibility guarantee [-Wpsabi]
>> /src/gcc/trunk/gcc/testsuite/gcc.dg/torture/pr78515.c:10:1: warning: GCC
>> vector passed by reference: non-standard ABI extension with no compatibility
>> guarantee [-Wpsabi]
>
> David has fixed this recently, but just for AIX.  Generally, -Wno-psabi
> is beneficial for all targets if it is needed on just one, I've committed
> following:
>
> 2016-12-15  Jakub Jelinek  <jakub@redhat.com>
>
>         * gcc.dg/tree-ssa/forwprop-35.c: Use -Wno-psabi everywhere.
>         * gcc.dg/torture/pr78515.c: Likewise.
>         * gcc.dg/pr69634.c: Likewise.

Thanks for the fixes and clarification.

There are a few more test cases with similar problems but I was unsure
if we wanted -Wno-psabi in the general options. I will add -Wno-psabi
to the additional test cases general options later today.

Thanks, David

Re: [PATCH] Fix PR78515

[Martin Jambor]

Hi,

On Fri, Nov 25, 2016 at 02:55:29PM +0100, Richard Biener wrote:
> On Fri, 25 Nov 2016, Martin Jambor wrote:
>
> ...
>
> > > There's still that odd 'stmt2'
> > > hanging around that gets set to sth else than stmt with
> > > 
> > >   op1 = gimple_assign_rhs1 (stmt);
> > > 
> > >   if (TREE_CODE (op1) == SSA_NAME)
> > >     {
> > >       if (SSA_NAME_IS_DEFAULT_DEF (op1))
> > >         index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1));
> > >       else
> > >         {
> > >           index = load_from_param (fbi, info->descriptors,
> > >                                    SSA_NAME_DEF_STMT (op1));
> > >           stmt2 = SSA_NAME_DEF_STMT (op1);
> > > 
> > > I assume that the original code wanted to restrict its processing
> > > to unary RHS of 'stmt' but still this "skips" arbitrary unary
> > > operations in 'stmt'?  But maybe I'm not understanding jump functions
> > > here.  If we have
> > > 
> > >   _2 = -param_1(D);
> > >   _3 = ~_2;
> > > 
> > > and stmt is _3 then we create a unary pass through JF with - (and the ~
> > > gets lost?).
> > >
> > 
> > It definitely looks like that.  Unary arithmetic jump functions have
> > been added only recently with the IPA VRP propagation and I remember
> > staring at the stmt2 thingy for a while during review but then
> > apparently I forgot about it.
> > 
> > It seems to me that the check should refer to stmt, I will do that and
> > see what breaks and how the IL looks at that point and then decide
> > where to go from there.
> 
> it's the only use of stmt2 though, so it must have been added for some
> reason... (maybe somebody wanted to handle simple copy-propagation?!).
> I'd say rip it out and thus keep stmt2 == stmt.  There must be
> a testcase added for this...
> 

So I have pondered about this some more and found out that while the
current code really makes no sense, it is fortunately harmless because
load_from_param will suceed only if it looks at a load from a
PARM_DECL that does not have an SSA_NAME and so cannot have any
arithmetic operation associated with it.  That means that there cannot
really be any difference between load_from_unmodified_param and
load_from_param and so the patch below re-unifies them.

It also removes the stmt2 variable from
compute_complex_assign_jump_func which means that the function is
actually more powerful now, able to do IPA-VRP on the added
testcase... which kind of makes me wonder whether it is appropriate at
this stage, but I'd prefer to put the code in order.

Bootstrapped and tested on x86_64-linux.  LTO-bootstrapped and testing
of the result still running on the same architecture.  OK for trunk if
it succeeds?  (The patch is intended to go on top of my fix for PR
79108).

Sorry for not spotting this when reviewing the patch that introduced
it in the first place,

Martin


2017-01-20  Martin Jambor  <mjambor@suse.cz>

	* ipa-prop.c (load_from_param_1): Removed.
	(load_from_unmodified_param): Bits from load_from_param_1 put back
	here.
	(load_from_param): Removed.
	(compute_complex_assign_jump_func): Removed stmt2 and just replaced it
	with stmt.  Reverted back to use of load_from_unmodified_param.

testsuite/
	* gcc.dg/ipa/vrp8.c: New test.
---
 gcc/ipa-prop.c                  | 68 ++++++++++-------------------------------
 gcc/testsuite/gcc.dg/ipa/vrp8.c | 42 +++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 52 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/vrp8.c

diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 4d77c9b25ef..512bcbed0cb 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -862,31 +862,6 @@ parm_preserved_before_stmt_p (struct ipa_func_body_info *fbi, int index,
   return !modified;
 }
 
-/* Main worker for load_from_unmodified_param and load_from_param.
-   If STMT is an assignment that loads a value from an parameter declaration,
-   return the index of the parameter in ipa_node_params.  Otherwise return -1.  */
-
-static int
-load_from_param_1 (struct ipa_func_body_info *fbi,
-		   vec<ipa_param_descriptor, va_gc> *descriptors,
-		   gimple *stmt)
-{
-  int index;
-  tree op1;
-
-  gcc_checking_assert (is_gimple_assign (stmt));
-  op1 = gimple_assign_rhs1 (stmt);
-  if (TREE_CODE (op1) != PARM_DECL)
-    return -1;
-
-  index = ipa_get_param_decl_index_1 (descriptors, op1);
-  if (index < 0
-      || !parm_preserved_before_stmt_p (fbi, index, stmt, op1))
-    return -1;
-
-  return index;
-}
-
 /* If STMT is an assignment that loads a value from an parameter declaration,
    return the index of the parameter in ipa_node_params which has not been
    modified.  Otherwise return -1.  */
@@ -896,29 +871,22 @@ load_from_unmodified_param (struct ipa_func_body_info *fbi,
 			    vec<ipa_param_descriptor, va_gc> *descriptors,
 			    gimple *stmt)
 {
+  int index;
+  tree op1;
+
   if (!gimple_assign_single_p (stmt))
     return -1;
 
-  return load_from_param_1 (fbi, descriptors, stmt);
-}
-
-/* If STMT is an assignment that loads a value from an parameter declaration,
-   return the index of the parameter in ipa_node_params.  Otherwise return -1.  */
-
-static int
-load_from_param (struct ipa_func_body_info *fbi,
-		 vec<ipa_param_descriptor, va_gc> *descriptors,
-		 gimple *stmt)
-{
-  if (!is_gimple_assign (stmt))
+  op1 = gimple_assign_rhs1 (stmt);
+  if (TREE_CODE (op1) != PARM_DECL)
     return -1;
 
-  enum tree_code rhs_code = gimple_assign_rhs_code (stmt);
-  if ((get_gimple_rhs_class (rhs_code) != GIMPLE_SINGLE_RHS)
-      && (get_gimple_rhs_class (rhs_code) != GIMPLE_UNARY_RHS))
+  index = ipa_get_param_decl_index_1 (descriptors, op1);
+  if (index < 0
+      || !parm_preserved_before_stmt_p (fbi, index, stmt, op1))
     return -1;
 
-  return load_from_param_1 (fbi, descriptors, stmt);
+  return index;
 }
 
 /* Return true if memory reference REF (which must be a load through parameter
@@ -1154,7 +1122,6 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi,
   tree op1, tc_ssa, base, ssa;
   bool reverse;
   int index;
-  gimple *stmt2 = stmt;
 
   op1 = gimple_assign_rhs1 (stmt);
 
@@ -1163,16 +1130,13 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi,
       if (SSA_NAME_IS_DEFAULT_DEF (op1))
 	index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1));
       else
-	{
-	  index = load_from_param (fbi, info->descriptors,
-				   SSA_NAME_DEF_STMT (op1));
-	  stmt2 = SSA_NAME_DEF_STMT (op1);
-	}
+	index = load_from_unmodified_param (fbi, info->descriptors,
+					    SSA_NAME_DEF_STMT (op1));
       tc_ssa = op1;
     }
   else
     {
-      index = load_from_param (fbi, info->descriptors, stmt);
+      index = load_from_unmodified_param (fbi, info->descriptors, stmt);
       tc_ssa = gimple_assign_lhs (stmt);
     }
 
@@ -1202,11 +1166,11 @@ compute_complex_assign_jump_func (struct ipa_func_body_info *fbi,
 	    break;
 	  }
 	case GIMPLE_UNARY_RHS:
-	  if (is_gimple_assign (stmt2)
-	      && gimple_assign_rhs_class (stmt2) == GIMPLE_UNARY_RHS
-	      && ! CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (stmt2)))
+	  if (is_gimple_assign (stmt)
+	      && gimple_assign_rhs_class (stmt) == GIMPLE_UNARY_RHS
+	      && ! CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (stmt)))
 	    ipa_set_jf_unary_pass_through (jfunc, index,
-					   gimple_assign_rhs_code (stmt2));
+					   gimple_assign_rhs_code (stmt));
 	default:;
 	}
       return;
diff --git a/gcc/testsuite/gcc.dg/ipa/vrp8.c b/gcc/testsuite/gcc.dg/ipa/vrp8.c
new file mode 100644
index 00000000000..55832b0886e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/vrp8.c
@@ -0,0 +1,42 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-ipa-cp-details" } */
+
+volatile int cond;
+int abs (int);
+
+volatile int g;
+
+int __attribute__((noinline, noclone))
+take_address (int *p)
+{
+  g = *p;
+}
+
+static int __attribute__((noinline, noclone))
+foo (int i)
+{
+  if (i < 5)
+    __builtin_abort ();
+  return 0;
+}
+
+static int __attribute__((noinline, noclone))
+bar (int j)
+{
+  foo (~j);
+  foo (abs (j));
+  foo (j);
+  take_address (&j);
+  return 0;
+}
+
+int
+main ()
+{
+  for (unsigned int i = 0; i < 10; ++i)
+    bar (i);
+
+  return 0;
+}
+
+/* { dg-final { scan-ipa-dump-times "Setting value range of param 0 \\\[-10, 9\\\]" 1 "cp" } } */
-- 
2.11.0

Re: [PATCH] Fix PR78515

[Richard Biener]

On January 20, 2017 11:27:04 PM GMT+01:00, Martin Jambor <mjambor@suse.cz> wrote:
>Hi,
>
>On Fri, Nov 25, 2016 at 02:55:29PM +0100, Richard Biener wrote:
>> On Fri, 25 Nov 2016, Martin Jambor wrote:
>>
>> ...
>>
>> > > There's still that odd 'stmt2'
>> > > hanging around that gets set to sth else than stmt with
>> > > 
>> > >   op1 = gimple_assign_rhs1 (stmt);
>> > > 
>> > >   if (TREE_CODE (op1) == SSA_NAME)
>> > >     {
>> > >       if (SSA_NAME_IS_DEFAULT_DEF (op1))
>> > >         index = ipa_get_param_decl_index (info, SSA_NAME_VAR
>(op1));
>> > >       else
>> > >         {
>> > >           index = load_from_param (fbi, info->descriptors,
>> > >                                    SSA_NAME_DEF_STMT (op1));
>> > >           stmt2 = SSA_NAME_DEF_STMT (op1);
>> > > 
>> > > I assume that the original code wanted to restrict its processing
>> > > to unary RHS of 'stmt' but still this "skips" arbitrary unary
>> > > operations in 'stmt'?  But maybe I'm not understanding jump
>functions
>> > > here.  If we have
>> > > 
>> > >   _2 = -param_1(D);
>> > >   _3 = ~_2;
>> > > 
>> > > and stmt is _3 then we create a unary pass through JF with - (and
>the ~
>> > > gets lost?).
>> > >
>> > 
>> > It definitely looks like that.  Unary arithmetic jump functions
>have
>> > been added only recently with the IPA VRP propagation and I
>remember
>> > staring at the stmt2 thingy for a while during review but then
>> > apparently I forgot about it.
>> > 
>> > It seems to me that the check should refer to stmt, I will do that
>and
>> > see what breaks and how the IL looks at that point and then decide
>> > where to go from there.
>> 
>> it's the only use of stmt2 though, so it must have been added for
>some
>> reason... (maybe somebody wanted to handle simple
>copy-propagation?!).
>> I'd say rip it out and thus keep stmt2 == stmt.  There must be
>> a testcase added for this...
>> 
>
>So I have pondered about this some more and found out that while the
>current code really makes no sense, it is fortunately harmless because
>load_from_param will suceed only if it looks at a load from a
>PARM_DECL that does not have an SSA_NAME and so cannot have any
>arithmetic operation associated with it.  That means that there cannot
>really be any difference between load_from_unmodified_param and
>load_from_param and so the patch below re-unifies them.
>
>It also removes the stmt2 variable from
>compute_complex_assign_jump_func which means that the function is
>actually more powerful now, able to do IPA-VRP on the added
>testcase... which kind of makes me wonder whether it is appropriate at
>this stage, but I'd prefer to put the code in order.
>
>Bootstrapped and tested on x86_64-linux.  LTO-bootstrapped and testing
>of the result still running on the same architecture.  OK for trunk if
>it succeeds?  (The patch is intended to go on top of my fix for PR
>79108).

OK.

Richard.

>
>Sorry for not spotting this when reviewing the patch that introduced
>it in the first place,
>
>Martin
>
>
>2017-01-20  Martin Jambor  <mjambor@suse.cz>
>
>	* ipa-prop.c (load_from_param_1): Removed.
>	(load_from_unmodified_param): Bits from load_from_param_1 put back
>	here.
>	(load_from_param): Removed.
>	(compute_complex_assign_jump_func): Removed stmt2 and just replaced it
>	with stmt.  Reverted back to use of load_from_unmodified_param.
>
>testsuite/
>	* gcc.dg/ipa/vrp8.c: New test.
>---
>gcc/ipa-prop.c                  | 68
>++++++++++-------------------------------
> gcc/testsuite/gcc.dg/ipa/vrp8.c | 42 +++++++++++++++++++++++++
> 2 files changed, 58 insertions(+), 52 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/vrp8.c
>
>diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
>index 4d77c9b25ef..512bcbed0cb 100644
>--- a/gcc/ipa-prop.c
>+++ b/gcc/ipa-prop.c
>@@ -862,31 +862,6 @@ parm_preserved_before_stmt_p (struct
>ipa_func_body_info *fbi, int index,
>   return !modified;
> }
> 
>-/* Main worker for load_from_unmodified_param and load_from_param.
>-   If STMT is an assignment that loads a value from an parameter
>declaration,
>-   return the index of the parameter in ipa_node_params.  Otherwise
>return -1.  */
>-
>-static int
>-load_from_param_1 (struct ipa_func_body_info *fbi,
>-		   vec<ipa_param_descriptor, va_gc> *descriptors,
>-		   gimple *stmt)
>-{
>-  int index;
>-  tree op1;
>-
>-  gcc_checking_assert (is_gimple_assign (stmt));
>-  op1 = gimple_assign_rhs1 (stmt);
>-  if (TREE_CODE (op1) != PARM_DECL)
>-    return -1;
>-
>-  index = ipa_get_param_decl_index_1 (descriptors, op1);
>-  if (index < 0
>-      || !parm_preserved_before_stmt_p (fbi, index, stmt, op1))
>-    return -1;
>-
>-  return index;
>-}
>-
>/* If STMT is an assignment that loads a value from an parameter
>declaration,
>return the index of the parameter in ipa_node_params which has not been
>    modified.  Otherwise return -1.  */
>@@ -896,29 +871,22 @@ load_from_unmodified_param (struct
>ipa_func_body_info *fbi,
> 			    vec<ipa_param_descriptor, va_gc> *descriptors,
> 			    gimple *stmt)
> {
>+  int index;
>+  tree op1;
>+
>   if (!gimple_assign_single_p (stmt))
>     return -1;
> 
>-  return load_from_param_1 (fbi, descriptors, stmt);
>-}
>-
>-/* If STMT is an assignment that loads a value from an parameter
>declaration,
>-   return the index of the parameter in ipa_node_params.  Otherwise
>return -1.  */
>-
>-static int
>-load_from_param (struct ipa_func_body_info *fbi,
>-		 vec<ipa_param_descriptor, va_gc> *descriptors,
>-		 gimple *stmt)
>-{
>-  if (!is_gimple_assign (stmt))
>+  op1 = gimple_assign_rhs1 (stmt);
>+  if (TREE_CODE (op1) != PARM_DECL)
>     return -1;
> 
>-  enum tree_code rhs_code = gimple_assign_rhs_code (stmt);
>-  if ((get_gimple_rhs_class (rhs_code) != GIMPLE_SINGLE_RHS)
>-      && (get_gimple_rhs_class (rhs_code) != GIMPLE_UNARY_RHS))
>+  index = ipa_get_param_decl_index_1 (descriptors, op1);
>+  if (index < 0
>+      || !parm_preserved_before_stmt_p (fbi, index, stmt, op1))
>     return -1;
> 
>-  return load_from_param_1 (fbi, descriptors, stmt);
>+  return index;
> }
> 
>/* Return true if memory reference REF (which must be a load through
>parameter
>@@ -1154,7 +1122,6 @@ compute_complex_assign_jump_func (struct
>ipa_func_body_info *fbi,
>   tree op1, tc_ssa, base, ssa;
>   bool reverse;
>   int index;
>-  gimple *stmt2 = stmt;
> 
>   op1 = gimple_assign_rhs1 (stmt);
> 
>@@ -1163,16 +1130,13 @@ compute_complex_assign_jump_func (struct
>ipa_func_body_info *fbi,
>       if (SSA_NAME_IS_DEFAULT_DEF (op1))
> 	index = ipa_get_param_decl_index (info, SSA_NAME_VAR (op1));
>       else
>-	{
>-	  index = load_from_param (fbi, info->descriptors,
>-				   SSA_NAME_DEF_STMT (op1));
>-	  stmt2 = SSA_NAME_DEF_STMT (op1);
>-	}
>+	index = load_from_unmodified_param (fbi, info->descriptors,
>+					    SSA_NAME_DEF_STMT (op1));
>       tc_ssa = op1;
>     }
>   else
>     {
>-      index = load_from_param (fbi, info->descriptors, stmt);
>+      index = load_from_unmodified_param (fbi, info->descriptors,
>stmt);
>       tc_ssa = gimple_assign_lhs (stmt);
>     }
> 
>@@ -1202,11 +1166,11 @@ compute_complex_assign_jump_func (struct
>ipa_func_body_info *fbi,
> 	    break;
> 	  }
> 	case GIMPLE_UNARY_RHS:
>-	  if (is_gimple_assign (stmt2)
>-	      && gimple_assign_rhs_class (stmt2) == GIMPLE_UNARY_RHS
>-	      && ! CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (stmt2)))
>+	  if (is_gimple_assign (stmt)
>+	      && gimple_assign_rhs_class (stmt) == GIMPLE_UNARY_RHS
>+	      && ! CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (stmt)))
> 	    ipa_set_jf_unary_pass_through (jfunc, index,
>-					   gimple_assign_rhs_code (stmt2));
>+					   gimple_assign_rhs_code (stmt));
> 	default:;
> 	}
>       return;
>diff --git a/gcc/testsuite/gcc.dg/ipa/vrp8.c
>b/gcc/testsuite/gcc.dg/ipa/vrp8.c
>new file mode 100644
>index 00000000000..55832b0886e
>--- /dev/null
>+++ b/gcc/testsuite/gcc.dg/ipa/vrp8.c
>@@ -0,0 +1,42 @@
>+/* { dg-do compile } */
>+/* { dg-options "-O2 -fdump-ipa-cp-details" } */
>+
>+volatile int cond;
>+int abs (int);
>+
>+volatile int g;
>+
>+int __attribute__((noinline, noclone))
>+take_address (int *p)
>+{
>+  g = *p;
>+}
>+
>+static int __attribute__((noinline, noclone))
>+foo (int i)
>+{
>+  if (i < 5)
>+    __builtin_abort ();
>+  return 0;
>+}
>+
>+static int __attribute__((noinline, noclone))
>+bar (int j)
>+{
>+  foo (~j);
>+  foo (abs (j));
>+  foo (j);
>+  take_address (&j);
>+  return 0;
>+}
>+
>+int
>+main ()
>+{
>+  for (unsigned int i = 0; i < 10; ++i)
>+    bar (i);
>+
>+  return 0;
>+}
>+
>+/* { dg-final { scan-ipa-dump-times "Setting value range of param 0
>\\\[-10, 9\\\]" 1 "cp" } } */