diff mbox

Fix PR78515

Message ID 20170120222704.4hbkmvjthvwkdp5y@virgil.suse.cz
State New
Headers show

Commit Message

Martin Jambor Jan. 20, 2017, 10:27 p.m. UTC 
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

Comments

Richard Biener Jan. 21, 2017, 8:20 a.m. UTC | #1 
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" } } */
diff mbox

Patch

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" } } */