Re: [PATCH PR90078]Capping comp_cost computation in ivopts

["Bin.Cheng" <amker.cheng@gmail.com>]

On Wed, Apr 17, 2019 at 3:10 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Apr 17, 2019 at 02:13:12PM +0800, bin.cheng wrote:
> > Hi,
> > As discussed in PR90078, this patch checks possible infinite_cost overflow in ivopts.
> > Also as discussed, overflow happens mostly because of cost scaling wrto bb_freq/loop_freq.
> > For the moment, we only implement capping in comp_cost operators, while in next
> > stage1, we may instead implement capping in get_scaled_computation_cost_at with
> > more supporting benchmark data.
> >
> > BTW, I think switching costs around comparison between infinite_cost is unnecessary
> > since there will be no overflow in integer after capping with infinite_cost.
> >
> > Bootstrap and test on x86_64, is it OK?
> >
> > Thanks,
> > bin
> >
> > 2019-04-17  Bin Cheng  <bin.cheng@linux.alibaba.com>
> >
> >         PR tree-optimization/92078
> >         * tree-ssa-loop-ivopts.c (comp_cost::operator +,-,+=,-+,/=,*=): Add
> >         checks for infinite_cost overflow.
> >
> > 2018-04-17  Bin Cheng  <bin.cheng@linux.alibaba.com>
> >
> >         PR tree-optimization/92078
> >         * gcc/testsuite/g++.dg/tree-ssa/pr90078.C: New test.
>
> --- a/gcc/tree-ssa-loop-ivopts.c
> +++ b/gcc/tree-ssa-loop-ivopts.c
> @@ -243,6 +243,9 @@ operator+ (comp_cost cost1, comp_cost cost2)
>    if (cost1.infinite_cost_p () || cost2.infinite_cost_p ())
>      return infinite_cost;
>
> +  if (cost1.cost + cost2.cost >= infinite_cost.cost)
> +    return infinite_cost;
>
> As
> #define INFTY 10000000
> what is the reason to keep the previous condition as well?
> I mean, if cost1.cost == INFTY or cost2.cost == INFTY,
> cost1.cost + cost2.cost >= INFTY too.
> Unless costs can go negative.
It's a bit complicated, but in general, costs can go negative.

>
> @@ -256,6 +259,8 @@ operator- (comp_cost cost1, comp_cost cost2)
>      return infinite_cost;
>
>    gcc_assert (!cost2.infinite_cost_p ());
> +  if (cost1.cost - cost2.cost >= infinite_cost.cost)
> +    return infinite_cost;
>
> Unless costs can be negative, when you first bail out
> for cost1.cost == INFTY, then cost1.cost - cost2.cost won't
> be INFTY (but could get negative).  So shouldn't there be a guard against
> that instead?  Or, if costs can be negative, shouldn't there be also
> guards that it doesn't grow too negative (say smaller than -INFTY)?
Negative cost is kind of a result of booking cost cancellation at
different place.  For example, it mostly comes from in modeling auto
increment/decrement addressing mode.  To be specific, when IV's
increment instruction can be merged into addressing mode, we cancel
cost of IV increment operation in cand-use cost.  Very likely 4 will
be subtracted.  In general, we wouldn't expect negative cost can go
too big, so there is no -INFTY logic in ivopts at all.  So this is the
least invasive fix for the moment, I would consider capping
bb_freq/loop_freq in the future which should rule out the overflow
possibility in the first place.

Thanks,
bin
>
>         Jakub