On January 20, 2017 11:27:04 PM GMT+01:00, Martin Jambor <mjam...@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 <mjam...@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" } } */
