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?
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
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 8f59f05ad21ac218834547593a7f308b4f837b1e 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: Iff93a1bd58d12e5e6951bc15ebf5db2ec2c85c2e
---
gcc/tree-scalar-evolution.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c
index 4b0ec02..d992582 100644
--- a/gcc/tree-scalar-evolution.c
+++ b/gcc/tree-scalar-evolution.c
@@ -3508,6 +3508,13 @@ 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)))
+ || 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
