Re: [PATCH PR90078]Capping comp_cost computation in ivopts

[Jakub Jelinek <jakub@redhat.com>]

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.

@@ -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)?

	Jakub