On Mon, May 6, 2019 at 12:24 PM Bin.Cheng <amker.ch...@gmail.com> wrote: > > On Mon, May 6, 2019 at 6:11 PM Richard Biener > <richard.guent...@gmail.com> wrote: > > > > On Sun, May 5, 2019 at 8:03 AM bin.cheng <bin.ch...@linux.alibaba.com> > > wrote: > > > > > > > ------------------------------------------------------------------ > > > > Sender:Jakub Jelinek <ja...@redhat.com> > > > > Sent At:2019 Apr. 17 (Wed.) 19:27 > > > > Recipient:Bin.Cheng <amker.ch...@gmail.com> > > > > Cc:bin.cheng <bin.ch...@linux.alibaba.com>; GCC Patches > > > > <gcc-patches@gcc.gnu.org> > > > > Subject:Re: [PATCH PR90078]Capping comp_cost computation in ivopts > > > > > > > > > > > > On Wed, Apr 17, 2019 at 07:14:05PM +0800, Bin.Cheng wrote: > > > > > > 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. > > > > > > > > Ok, no objections from me then (but as I don't know anything about it, > > > > not an ack either; you are ivopts maintainer, so you don't need one). > > > > > > Hi, > > > The previous patch was reverted on GCC-9 because of PR90240. PR90240 is > > > now > > > fixed by another patch. This is the updated patch for PR90078. It > > > promotes type > > > of ivopts cost from int to int64_t, as well as change behavior of > > > infinite_cost overflow > > > from saturation to assert. > > > Please note, implicit conversions are kept in cost computation as before > > > without > > > introducing any narrowing. > > > > > > Bootstrap/test on x86_64 along with PR90240 patch. Is it OK? > > > > Do not include system headers in .c files, instead those need to be > > (and are already) > > included via system.h. > > > > /* The infinite cost. */ > > -#define INFTY 10000000 > > +#define INFTY 1000000000L > > > > do we actually need this? What happens on a ilp32 host? That is, I believe > > you can drop the 'L' (it fits into an int anyways) > Yeah, now I think if int64_t is necessary or not. With the scaling > bound and assertions. > > > > @@ -256,6 +259,7 @@ operator- (comp_cost cost1, comp_cost cost2) > > return infinite_cost; > > > > gcc_assert (!cost2.infinite_cost_p ()); > > + gcc_assert (cost1.cost - cost2.cost < infinite_cost.cost); > > > > cost1.cost -= cost2.cost; > > cost1.complexity -= cost2.complexity; > > > > probably a pre-existing issue, but we do not seem to handle underflow > > here in general, nor check that underflow doesn't get us below -INFTY. > > > > I guess we really don't want negative costs? That doesn't seem to be > > documented and I was also wondering why the cost isn't unsigned... > > > > @@ -638,7 +646,7 @@ struct iv_ca > > comp_cost cand_use_cost; > > > > /* Total cost of candidates. */ > > - unsigned cand_cost; > > + int64_t cand_cost; > > > > /* Number of times each invariant variable is used. */ > > unsigned *n_inv_var_uses; > > > > shows this "issue". Can't we use uint64_t throughout the patch? > Oh, it's actually explained in previous message, > https://gcc.gnu.org/ml/gcc-patches/2019-04/msg00697.html > In short, yes the cost can be negative. The negative cost should be > small in absolute value, and we don't check -INFTY. With the scaling > bound change, I don't think -INFTY is possible IIUC.
Ah, I see. The patch is OK then with 1000000000L in the #define changed to just 1000000000 Richard. > Thanks, > bin > > > > Otherwise this looks OK. > > > > Thanks, > > Richard. > > > > > Thanks, > > > bin > > > 2019-05-05 Bin Cheng <bin.ch...@linux.alibaba.com> > > > > > > PR tree-optimization/90078 > > > * tree-ssa-loop-ivopts.c (inttypes.h): Include new header file. > > > (INFTY): Increase the value for infinite cost. > > > (struct comp_cost): Promote type of members to int64_t. > > > (infinite_cost): Don't set complexity in initialization. > > > (comp_cost::operator +,-,+=,-+,/=,*=): Assert when cost > > > computation > > > overflows to infinite_cost. > > > (adjust_setup_cost): Promote type of parameter and cost > > > computation > > > to int64_t. > > > (struct ainc_cost_data, struct iv_ca): Promote type of member to > > > int64_t. > > > (get_scaled_computation_cost_at, determine_iv_cost): Promote type > > > of > > > cost computation to int64_t. > > > (determine_group_iv_costs, iv_ca_dump, find_optimal_iv_set): Use > > > int64_t's format specifier in dump. > > > > > > 2018-05-05 Bin Cheng <bin.ch...@linux.alibaba.com> > > > > > > PR tree-optimization/90078 > > > * g++.dg/tree-ssa/pr90078.C: New test.
