Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Jul 5, 2018 at 1:02 PM Kugan Vivekanandarajah
<kugan.vivekanandarajah@linaro.org> wrote:
>
> Hi Richard,
>
> Thanks for the review.
>
> On 28 June 2018 at 21:26, Richard Biener <richard.guenther@gmail.com> wrote:
> > On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah
> > <kugan.vivekanandarajah@linaro.org> wrote:
> >>
> >> Hi Richard,
> >>
> >> Thanks for the review.
> >>
> >> On 25 June 2018 at 20:01, Richard Biener <richard.guenther@gmail.com> wrote:
> >> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah
> >> > <kugan.vivekanandarajah@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 patch is OK.

> >
> > 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.

I couldn't find it quickly, asked Jakub now.

Thanks,
Richard.

>
> 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  <kuganv@linaro.org>
> >> >>
> >> >>     * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.