Re: [PATCH PR90078]Capping comp_cost computation in ivopts

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

On Mon, May 6, 2019 at 6:11 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Sun, May 5, 2019 at 8:03 AM bin.cheng <bin.cheng@linux.alibaba.com> wrote:
> >
> > > ------------------------------------------------------------------
> > > Sender:Jakub Jelinek <jakub@redhat.com>
> > > Sent At:2019 Apr. 17 (Wed.) 19:27
> > > Recipient:Bin.Cheng <amker.cheng@gmail.com>
> > > Cc:bin.cheng <bin.cheng@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.

Thanks,
bin
>
> Otherwise this looks OK.
>
> Thanks,
> Richard.
>
> > Thanks,
> > bin
> > 2019-05-05  Bin Cheng  <bin.cheng@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.cheng@linux.alibaba.com>
> >
> >         PR tree-optimization/90078
> >         * g++.dg/tree-ssa/pr90078.C: New test.