Hi Richard,
Thanks for the review.
On 28 June 2018 at 21:26, Richard Biener <richard.guent...@gmail.com> wrote:
> On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah
> <kugan.vivekanandara...@linaro.org> wrote:
>>
>> Hi Richard,
>>
>> Thanks for the review.
>>
>> On 25 June 2018 at 20:01, Richard Biener <richard.guent...@gmail.com> wrote:
>> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah
>> > <kugan.vivekanandara...@linaro.org> wrote:
>> >>
>> >> [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
>> >
>> > This says that COND_EXPR itself isn't expensive. I think we should
>> > constrain that a bit.
>> > I think a good default would be to only allow a single COND_EXPR which
>> > you can achieve
>> > by adding a bool in_cond_expr_p = false argument to the function, pass
>> > in_cond_expr_p
>> > down and pass true down from the COND_EXPR handling itself.
>> >
>> > I'm not sure if we should require either COND_EXPR arm (operand 1 or
>> > 2) to be constant
>> > or !EXPR_P (then multiple COND_EXPRs might be OK).
>> >
>> > The main idea is to avoid evaluating many expressions but only
>> > choosing one in the end.
>> >
>> > The simplest patch achieving that is sth like
>> >
>> > + if (code == COND_EXPR)
>> > + return (expression_expensive_p (TREE_OPERAND (expr, 0))
>> > || (EXPR_P (TREE_OPERAND (expr, 1)) && EXPR_P
>> > (TREE_OPERAND (expr, 2)))
>> > + || expression_expensive_p (TREE_OPERAND (expr, 1))
>> > + || expression_expensive_p (TREE_OPERAND (expr, 2)));
>> >
>> > OK with that change.
>>
>> Is || (EXPR_P (TREE_OPERAND (expr, 1)) || EXPR_P (TREE_OPERAND (expr,
>> 2))) slightly better ?
>> Attaching with the change. Is this OK?
>
> Well, it won't allow x != 0 ? popcount (x) : 1 because popcount(x) is
> CALL_EXPR.
>
>>
>>
>> Because, for pr81661.c, we now allow as not expensive
>> <plus_expr 0x7ffff6a87b40
>> type <integer_type 0x7ffff69455e8 int public SI
>> size <integer_cst 0x7ffff692df30 constant 32>
>> unit-size <integer_cst 0x7ffff692df48 constant 4>
>> align:32 warn_if_not_align:0 symtab:0 alias-set 1
>> canonical-type 0x7ffff69455e8 precision:32 min <integer_cst
>> 0x7ffff692dee8 -2147483648> max <integer_cst 0x7ffff692df00
>> 2147483647>
>> pointer_to_this <pointer_type 0x7ffff694d9d8>>
>>
>> arg:0 <plus_expr 0x7ffff6a87b68 type <integer_type 0x7ffff69455e8 int>
>>
>> arg:0 <ssa_name 0x7ffff6937bd0 type <integer_type 0x7ffff69455e8 int>
>> visited
>> def_stmt a.1_10 = a;
>> version:10>
>> arg:1 <integer_cst 0x7ffff694a0d8 constant -1>>
>> arg:1 <cond_expr 0x7ffff6a89000 type <integer_type 0x7ffff69455e8 int>
>>
>> arg:0 <ge_expr 0x7ffff6a87b90 type <boolean_type 0x7ffff6945b28
>> _Bool>
>>
>> arg:0 <plus_expr 0x7ffff6a87bb8 type <integer_type
>> 0x7ffff69455e8 int>
>>
>> arg:0 <plus_expr 0x7ffff6a87be0 type <integer_type
>> 0x7ffff69455e8 int>
>> arg:0 <ssa_name 0x7ffff6937bd0> arg:1 <integer_cst
>> 0x7ffff694a0d8 -1>>
>> arg:1 <ssa_name 0x7ffff6937c18 type <integer_type
>> 0x7ffff69455e8 int>
>> visited
>> def_stmt c.2_11 = c;
>> version:11>>
>> arg:1 <ssa_name 0x7ffff6937ca8 type <integer_type
>> 0x7ffff69455e8 int>
>> visited
>> def_stmt b.3_13 = b;
>> version:13>>
>> arg:1 <negate_expr 0x7ffff6a88560 type <integer_type 0x7ffff69455e8
>> int>
>>
>> arg:0 <nop_expr 0x7ffff6a88580 type <integer_type
>> 0x7ffff69455e8 int>
>>
>> arg:0 <minus_expr 0x7ffff6a87c08 type <integer_type
>> 0x7ffff6a55b28>
>>
>> arg:0 <nop_expr 0x7ffff6a885a0 type <integer_type
>> 0x7ffff6a55b28>
>>
>> arg:0 <plus_expr 0x7ffff6a87c30 type
>> <integer_type 0x7ffff69455e8 int>
>> arg:0 <plus_expr 0x7ffff6a87c58> arg:1
>> <ssa_name 0x7ffff6937c18>>>
>> arg:1 <nop_expr 0x7ffff6a885c0 type <integer_type
>> 0x7ffff6a55b28>
>> arg:0 <ssa_name 0x7ffff6937ca8>>>>>
>> arg:2 <integer_cst 0x7ffff694a090 constant 0>>>
>>
>> Which also leads to an ICE in gimplify_modify_expr. I think this is a
>> latent issue and I am trying to find the source
>
> Well, I think that's because some COND_EXPRs only gimplify to
> conditional code. See gimplify_cond_expr:
>
> if (gimplify_ctxp->allow_rhs_cond_expr
> /* If either branch has side effects or could trap, it can't be
> evaluated unconditionally. */
> && !TREE_SIDE_EFFECTS (then_)
> && !generic_expr_could_trap_p (then_)
> && !TREE_SIDE_EFFECTS (else_)
> && !generic_expr_could_trap_p (else_))
> return gimplify_pure_cond_expr (expr_p, pre_p);
>
> so we probably have to treat TREE_SIDE_EFFECTS / generic_expr_could_trap_p as
> "expensive" as well for the purpose of final value replacement unless we are
> going to support a code-generation way different from gimplification.
Is the attached patch which does this is OK?. I had to fix couple of
testcases because now the final value replacement removed the loop for
pr64183.c and pr85073.c is popcount pattern so I just disabled it so
that we can test what was tested earlier.
>
> The testcase you cite uses -ftrapv which is why we run into this issue. Note
> that final value replacement deals with this by rewriting the expression to
> unsigned but it does so only after gimplification. IIRC Jakub recently
> added a helper to rewrite GENERIC to unsigned so that might be useful
> in this context.
Could you kindly refer me to Jakubs patch please.
Thanks,
Kugan
>
> Richard.
>
>> the expr in gimple_modify_expr is
>> <modify_expr 0x7ffff6a87cd0
>> type <integer_type 0x7ffff69455e8 int public SI
>> size <integer_cst 0x7ffff692df30 constant 32>
>> unit-size <integer_cst 0x7ffff692df48 constant 4>
>> align:32 warn_if_not_align:0 symtab:0 alias-set 1
>> canonical-type 0x7ffff69455e8 precision:32 min <integer_cst
>> 0x7ffff692dee8 -2147483648> max <integer_cst 0x7ffff692df00
>> 2147483647>
>> pointer_to_this <pointer_type 0x7ffff694d9d8>>
>> side-effects
>> arg:0 <var_decl 0x7ffff6a802d0 iftmp.5 type <integer_type
>> 0x7ffff69455e8 int>
>> used ignored SI (null):0:0 size <integer_cst 0x7ffff692df30
>> 32> unit-size <integer_cst 0x7ffff692df48 4>
>> align:32 warn_if_not_align:0 context <function_decl 0x7ffff6a57700
>> foo>>
>> arg:1 <negate_expr 0x7ffff6a88560 type <integer_type 0x7ffff69455e8 int>
>>
>> arg:0 <nop_expr 0x7ffff6a88580 type <integer_type 0x7ffff69455e8 int>
>>
>> arg:0 <minus_expr 0x7ffff6a87c08 type <integer_type
>> 0x7ffff6a55b28>
>>
>> arg:0 <nop_expr 0x7ffff6a885a0 type <integer_type
>> 0x7ffff6a55b28>
>>
>> arg:0 <plus_expr 0x7ffff6a87c30 type <integer_type
>> 0x7ffff69455e8 int>
>>
>> arg:0 <plus_expr 0x7ffff6a87c58 type
>> <integer_type 0x7ffff69455e8 int>
>> arg:0 <ssa_name 0x7ffff6937bd0> arg:1
>> <integer_cst 0x7ffff694a0d8 -1>>
>> arg:1 <ssa_name 0x7ffff6937c18 type
>> <integer_type 0x7ffff69455e8 int>
>> visited
>> def_stmt c.2_11 = c;
>> version:11>>>
>> arg:1 <nop_expr 0x7ffff6a885c0 type <integer_type
>> 0x7ffff6a55b28>
>>
>> arg:0 <ssa_name 0x7ffff6937ca8 type <integer_type
>> 0x7ffff69455e8 int>
>> visited
>> def_stmt b.3_13 = b;
>> version:13>>>>>>
>>
>> and the *to_p is not SSA_NAME and VAR_DECL.
>>
>> Thanks,
>> Kugan
>>
>>
>>
>> >
>> > Richard.
>> >
>> >> gcc/ChangeLog:
>> >>
>> >> 2018-06-22 Kugan Vivekanandarajah <kug...@linaro.org>
>> >>
>> >> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
From 9a9712c7ec4cc8dd3824ccb7ab441742b85bebbe Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org>
Date: Fri, 22 Jun 2018 14:10:26 +1000
Subject: [PATCH 1/3] generate popcount when checked for zero
Change-Id: I951e6d487268b757cbdaa8dcf671ab1377490db6
---
gcc/gimplify.c | 2 +-
gcc/gimplify.h | 1 +
gcc/testsuite/gcc.dg/tree-ssa/pr64183.c | 2 +-
gcc/testsuite/gcc.target/i386/pr85073.c | 2 +-
gcc/tree-scalar-evolution.c | 12 ++++++++++++
5 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 97543ed..4de98e5 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -3878,7 +3878,7 @@ gimplify_pure_cond_expr (tree *expr_p, gimple_seq *pre_p)
EXPR is GENERIC, while tree_could_trap_p can be called
only on GIMPLE. */
-static bool
+bool
generic_expr_could_trap_p (tree expr)
{
unsigned i, n;
diff --git a/gcc/gimplify.h b/gcc/gimplify.h
index dd0e4c0..62ca869 100644
--- a/gcc/gimplify.h
+++ b/gcc/gimplify.h
@@ -83,6 +83,7 @@ extern enum gimplify_status gimplify_arg (tree *, gimple_seq *, location_t,
extern void gimplify_function_tree (tree);
extern enum gimplify_status gimplify_va_arg_expr (tree *, gimple_seq *,
gimple_seq *);
+extern bool generic_expr_could_trap_p (tree expr);
gimple *gimplify_assign (tree, tree, gimple_seq *);
#endif /* GCC_GIMPLIFY_H */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr64183.c b/gcc/testsuite/gcc.dg/tree-ssa/pr64183.c
index 7a854fc..50d0c5a 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr64183.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr64183.c
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-O3 -fno-tree-vectorize -fdump-tree-cunroll-details" } */
+/* { dg-options "-O3 -fno-tree-vectorize -fdisable-tree-sccp -fdump-tree-cunroll-details" } */
int bits;
unsigned int size;
diff --git a/gcc/testsuite/gcc.target/i386/pr85073.c b/gcc/testsuite/gcc.target/i386/pr85073.c
index 187102d..71a5d23 100644
--- a/gcc/testsuite/gcc.target/i386/pr85073.c
+++ b/gcc/testsuite/gcc.target/i386/pr85073.c
@@ -1,6 +1,6 @@
/* PR target/85073 */
/* { dg-do compile } */
-/* { dg-options "-O2 -mbmi" } */
+/* { dg-options "-O2 -mbmi -fdisable-tree-sccp" } */
int
foo (unsigned x)
diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c
index 4b0ec02..8e29005 100644
--- a/gcc/tree-scalar-evolution.c
+++ b/gcc/tree-scalar-evolution.c
@@ -3508,6 +3508,18 @@ expression_expensive_p (tree expr)
return false;
}
+ if (code == COND_EXPR)
+ return (expression_expensive_p (TREE_OPERAND (expr, 0))
+ || (EXPR_P (TREE_OPERAND (expr, 1))
+ && EXPR_P (TREE_OPERAND (expr, 2)))
+ /* If either branch has side effects or could trap. */
+ || TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 1))
+ || generic_expr_could_trap_p (TREE_OPERAND (expr, 1))
+ || TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 0))
+ || generic_expr_could_trap_p (TREE_OPERAND (expr, 0))
+ || expression_expensive_p (TREE_OPERAND (expr, 1))
+ || expression_expensive_p (TREE_OPERAND (expr, 2)));
+
switch (TREE_CODE_CLASS (code))
{
case tcc_binary:
--
2.7.4
