From: Richard Biener <richard.guenther@gmail.com>
To: "Bin.Cheng" <amker.cheng@gmail.com>
Cc: "bin.cheng" <bin.cheng@linux.alibaba.com>,
Jakub Jelinek <jakub@redhat.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH PR90078]Capping comp_cost computation in ivopts
Date: Mon, 06 May 2019 10:27:00 -0000 [thread overview]
Message-ID: <CAFiYyc11T=XnCivVn7K=owYYW5ySu6agO_VQo5ykvt5eyXN1zg@mail.gmail.com> (raw)
In-Reply-To: <CAHFci29W+jg=KvY1nvf0TZqVzpYszxGQg-Sz3qDRkZNJYEGqmA@mail.gmail.com>
On Mon, May 6, 2019 at 12:24 PM Bin.Cheng <amker.cheng@gmail.com> wrote:
>
> 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.
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.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.
prev parent reply other threads:[~2019-05-06 10:27 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-17 7:00 bin.cheng
2019-04-17 7:19 ` Jakub Jelinek
2019-04-17 11:14 ` Bin.Cheng
2019-04-17 11:35 ` Jakub Jelinek
2019-05-05 6:02 ` bin.cheng
2019-05-06 10:11 ` Richard Biener
2019-05-06 10:24 ` Bin.Cheng
2019-05-06 10:27 ` Richard Biener [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc11T=XnCivVn7K=owYYW5ySu6agO_VQo5ykvt5eyXN1zg@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=amker.cheng@gmail.com \
--cc=bin.cheng@linux.alibaba.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).