This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Thu, Jul 5, 2018 at 1:29 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> 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.

It was rewrite_to_non_trapping_overflow available  in tree.h.  Thus
final value replacement
could use that before gimplifying instead of using rewrite_to_defined_overflow

Richard.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]