public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Aldy Hernandez <aldyh@redhat.com>
To: Richard Biener <richard.guenther@gmail.com>
Cc: Jeff Law <jeffreyalaw@gmail.com>,
	GCC patches <gcc-patches@gcc.gnu.org>,
	 Andrew MacLeod <amacleod@redhat.com>
Subject: Re: [RFC] Remove VRP threader passes in exchange for better threading pre-VRP.
Date: Tue, 19 Oct 2021 09:33:20 +0200	[thread overview]
Message-ID: <CAGm3qMWkH+wiVHYMDFkHSEcVF83Zjtv8W14HQcFc76ZpbP-2iA@mail.gmail.com> (raw)
In-Reply-To: <CAFiYyc2zWLJcZdbx25bFq=y9GKtPEjqYwps-ThFEY-Rr7uJDVA@mail.gmail.com>

On Tue, Oct 19, 2021 at 8:52 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Oct 18, 2021 at 4:03 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> >
> >
> > On 10/18/21 3:41 PM, Aldy Hernandez wrote:
> >
> > > I've been experimenting with reducing the total number of threading
> > > passes, and I'd like to see if there's consensus/stomach for altering
> > > the pipeline.  Note, that the goal is to remove forward threader clients,
> > > not the other way around.  So, we should prefer to remove a VRP threader
> > > instance over a *.thread one immediately before VRP.
> > >
> > > After some playing, it looks like if we enable fully-resolving mode in
> > > the *.thread passes immediately preceeding VRP, we can remove the VRP
> > > threading passes altogether, thus removing 2 threading passes (and
> > > forward threading passes at that!).
> >
> > It occurs to me that we could also remove the threading before VRP
> > passes, and enable a fully-resolving backward threader after VRP.  I
> > haven't played with this scenario, but it should be just as good.  That
> > being said, I don't know the intricacies of why we had both pre and post
> > VRP threading passes, and if one is ideally better than the other.
>
> It was done because they were different threaders.  Since the new threader
> uses built-in VRP it shouldn't really matter whether it's before or after
> VRP _for the threading_, but it might be that if threading runs before VRP
> then VRP itself can do a better job on cleaning up the IL.

Good point.

FWIW, earlier this season I played with replacing the VRPs with evrp
instances (which fold far more conditionals) and I found that the
threaders can actually find LESS opportunities after *vrp fold away
things.  I don't know if this is a good or a bad thing.  Perhaps we
should benchmark three alternatives:

1. Mainline
2. Fully resolving threader -> VRP -> No threading.
3. No threading -> VRP -> Full resolving threader.

...and see what the actual effect is, regardless of number of threaded paths.

Speak of which, what's the blessed way of benchmarking performance
nowadays?  I've seen some PRs fly that measure some more lightweight
benchmarks (open source?) than a full blown SPEC.

>
> +      /* ?? Is this still needed.  ?? */
>        /* Threading can leave many const/copy propagations in the IL.
>          Clean them up.  Instead of just copy_prop, we use ccp to
>          compute alignment and nonzero bits.  */
>
> Yes, it's still needed but not for the stated reason - the VRP
> substitution and folding stage should deal with copy/constant propagation
> but we replaced the former copy propagation with CCP to re-compute
> nonzero bits & alignment so I'd change the comment to
>
>    /* Run CCP to compute alignment and nonzero bits.  */

Ahh..

There's another similar comment after DOM.  Is this comment still relevant?

      NEXT_PASS (pass_dominator, true /* may_peel_loop_headers_p */);
      /* Threading can leave many const/copy propagations in the IL.
     Clean them up.  Failure to do so well can lead to false
     positives from warnings for erroneous code.  */
      NEXT_PASS (pass_copy_prop);
      /* Identify paths that should never be executed in a conforming
     program and isolate those paths.  */

Thanks.
Aldy


  reply	other threads:[~2021-10-19  7:33 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-18 13:41 Aldy Hernandez
2021-10-18 14:03 ` Aldy Hernandez
2021-10-19  6:52   ` Richard Biener
2021-10-19  7:33     ` Aldy Hernandez [this message]
2021-10-19  8:40       ` Richard Biener
2021-10-19  9:06         ` Aldy Hernandez
2021-10-19  9:16           ` Aldy Hernandez
2021-10-19 23:06       ` Jeff Law
2021-10-19 23:00   ` Jeff Law
2021-10-20  9:27     ` Aldy Hernandez
2021-10-20 12:32       ` Andrew MacLeod
2021-10-20 13:08         ` Aldy Hernandez
2021-10-20 17:55       ` Jeff Law
2021-10-19 22:58 ` Jeff Law

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=CAGm3qMWkH+wiVHYMDFkHSEcVF83Zjtv8W14HQcFc76ZpbP-2iA@mail.gmail.com \
    --to=aldyh@redhat.com \
    --cc=amacleod@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jeffreyalaw@gmail.com \
    --cc=richard.guenther@gmail.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).