This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix PR64878: do not jump thread across two loop iterations


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]