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 <[email protected]> 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
