This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Fix PR64878: do not jump thread across two loop iterations
- From: Sebastian Pop <sebpop at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Richard Biener <richard dot guenther at gmail dot com>, gcc-patches at gcc dot gnu dot org, Jeff Law <law at redhat dot com>
- Date: Thu, 5 Feb 2015 19:35:30 +0000
- Subject: Re: Fix PR64878: do not jump thread across two loop iterations
- Authentication-results: sourceware.org; auth=none
- References: <20150204212006 dot GA18747 at f1 dot c dot bardezibar dot internal> <899D7056-398A-45AD-8238-8A5323E5E00C at gmail dot com> <20150205144632 dot GH1746 at tucnak dot redhat dot com>
Jakub Jelinek wrote:
> On Thu, Feb 05, 2015 at 08:21:46AM +0100, Richard Biener wrote:
> > On February 4, 2015 10:20:06 PM CET, Sebastian Pop <sebpop@gmail.com> wrote:
> > >The attached patch stops the recursion in the detection of FSM
> > >jump-threads at
> > >loop phi nodes after having visited a loop phi node. This avoids
> > >jump-threading
> > >two iterations forward that were possible due to a flip-flop operation
> > >that
> > >exchange the value of the switch control variable as illustrated in the
> > >testcase:
> > >
> > >do {
> > > c = read_from_memory;
> > > switch (a) {
> > > case 0:
> > > if (c == ' ')
> > > [...]
> > > else
> > > a = b; // flip-flop
> > > break;
> > > case 1:
> > > a = 0;
> > > b = 15; // this will jump-thread to 15
> > > break;
> > >
> > > case 15:
> > > [...]
> > > }
> > >} while (...);
> > >
> > >The patch has passed bootstrap and regression testing on x86_64-linux.
> > >Ok to commit?
> >
> > But is sounds like a useful optimization?
>
> Only if we could do it reliably. If it isn't fixable in a different way for
> GCC 5.0, then I think it is better to live with a possibly
> missed-optimization than wrong-code.
The alternative to correct the code generation for this jump-thread is to add
code to detect control dependences ("a = b" is dependent on all the conditions
around it, "c != ' ' && c != '/'") and to copy the control dependences over in
the duplicated jump-thread path. I'm more confortable with the 3 line fix that
I submitted to disable the functionality than adding 100 lines to support copy
of control dependences. From a perf point of view, the patch only disables one
jump-thread in the testcase that was failing, all the other jump-threads are
still enabled, and we do not regress on coremark.
Does it make sense to also add a check on how many jumps are threaded for the
testcase that I'm adding with this patch?
Thanks,
Sebastian