| Message ID | 20141209173842.GA31443@f1.c.bardezibar.internal |
|---|---|
| State | New |
| Headers | show |
On 12/09/14 10:38, Sebastian Pop wrote: > Richard Biener wrote: >> On Mon, Dec 8, 2014 at 10:49 PM, Steve Ellcey <sellcey@mips.com> wrote: >>> expected? Should this test also check flag_thread_jumps? Or should >>> that be getting checked somewhere else? >> >> -fthread-jumps is an RTL optimization flag and ignored on GIMPLE. > > Does it make sense to add a -f[no-]tree-thread-jumps to enable/disable the tree > jump threading? I could also add -f[no-]tree-fsm-thread-jumps. Opinions? Our option handling is a bit of a mess if we look at it from the user standpoint. Given that the vast majority of jump threading happens on gimple, ISTM that -f[no-]thread-jumps ought to be controlling the gimple implementation. One could easily argue that the user doesn't really care about where in the pipeline the optimization is implemented. My vote would be to just make -fthread-jumps control both RTL and gimple jump threading. > > On the llvm test-suite, I have seen one ICE with my fsm jump-thread patch. > This patch fixes the problem: > > diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c > index 12f83ba..f8c736e 100644 > --- a/gcc/tree-ssa-threadupdate.c > +++ b/gcc/tree-ssa-threadupdate.c > @@ -2564,6 +2564,7 @@ thread_through_all_blocks (bool may_peel_loop_headers) > FOR_EACH_LOOP (loop, LI_FROM_INNERMOST) > { > if (!loop->header > + || !loop_latch_edge (loop) > || !bitmap_bit_p (threaded_blocks, loop->header->index)) > continue; > > retval |= thread_through_loop_header (loop, may_peel_loop_headers); > > Ok to commit after regstrap? This seems to be indicating that we have with no edge from the latch block to the header block. I'd like to know better how we got into that state. Also, a test for the GCC testsuite would be good. I have no idea what license covers the LLVM testsuite. But given a good analysis of the problem we may be able to write a suitable test independent of the LLVM test. jeff
On December 9, 2014 7:39:48 PM CET, Jeff Law <law@redhat.com> wrote: >On 12/09/14 10:38, Sebastian Pop wrote: >> Richard Biener wrote: >>> On Mon, Dec 8, 2014 at 10:49 PM, Steve Ellcey <sellcey@mips.com> >wrote: >>>> expected? Should this test also check flag_thread_jumps? Or >should >>>> that be getting checked somewhere else? >>> >>> -fthread-jumps is an RTL optimization flag and ignored on GIMPLE. >> >> Does it make sense to add a -f[no-]tree-thread-jumps to >enable/disable the tree >> jump threading? I could also add -f[no-]tree-fsm-thread-jumps. >Opinions? >Our option handling is a bit of a mess if we look at it from the user >standpoint. Given that the vast majority of jump threading happens on >gimple, ISTM that -f[no-]thread-jumps ought to be controlling the >gimple >implementation. One could easily argue that the user doesn't really >care about where in the pipeline the optimization is implemented. > >My vote would be to just make -fthread-jumps control both RTL and >gimple >jump threading. Works for me. > >> >> On the llvm test-suite, I have seen one ICE with my fsm jump-thread >patch. >> This patch fixes the problem: >> >> diff --git a/gcc/tree-ssa-threadupdate.c >b/gcc/tree-ssa-threadupdate.c >> index 12f83ba..f8c736e 100644 >> --- a/gcc/tree-ssa-threadupdate.c >> +++ b/gcc/tree-ssa-threadupdate.c >> @@ -2564,6 +2564,7 @@ thread_through_all_blocks (bool >may_peel_loop_headers) >> FOR_EACH_LOOP (loop, LI_FROM_INNERMOST) >> { >> if (!loop->header >> + || !loop_latch_edge (loop) >> || !bitmap_bit_p (threaded_blocks, loop->header->index)) >> continue; >> >> retval |= thread_through_loop_header (loop, >may_peel_loop_headers); >> >> Ok to commit after regstrap? >This seems to be indicating that we have with no edge from the latch >block to the header block. I'd like to know better how we got into >that >state. It Also returns null for loops with multiple latches. So the patch looks OK for me. Thanks, Richard. >Also, a test for the GCC testsuite would be good. I have no idea what >license covers the LLVM testsuite. But given a good analysis of the >problem we may be able to write a suitable test independent of the LLVM > >test. > >jeff
On 12/09/14 12:43, Richard Biener wrote: >> This seems to be indicating that we have with no edge from the latch >> block to the header block. I'd like to know better how we got into >> that >> state. > > It Also returns null for loops with multiple latches. So the patch looks OK for me. Ah, OK. Jeff
On Dec 9, 2014, at 10:39 AM, Jeff Law <law@redhat.com> wrote:
> Also, a test for the GCC testsuite would be good. I have no idea what license covers the LLVM testsuite. But given a good analysis of the problem we may be able to write a suitable test independent of the LLVM test.
So, the usual engineerings rules should work just fine on it. delta reduce and submit.
Richard Biener wrote: > > > > >> > >> On the llvm test-suite, I have seen one ICE with my fsm jump-thread > >patch. > >> This patch fixes the problem: > >> > >> diff --git a/gcc/tree-ssa-threadupdate.c > >b/gcc/tree-ssa-threadupdate.c > >> index 12f83ba..f8c736e 100644 > >> --- a/gcc/tree-ssa-threadupdate.c > >> +++ b/gcc/tree-ssa-threadupdate.c > >> @@ -2564,6 +2564,7 @@ thread_through_all_blocks (bool > >may_peel_loop_headers) > >> FOR_EACH_LOOP (loop, LI_FROM_INNERMOST) > >> { > >> if (!loop->header > >> + || !loop_latch_edge (loop) > >> || !bitmap_bit_p (threaded_blocks, loop->header->index)) > >> continue; > >> > >> retval |= thread_through_loop_header (loop, > >may_peel_loop_headers); > >> > >> Ok to commit after regstrap? > >This seems to be indicating that we have with no edge from the latch > >block to the header block. I'd like to know better how we got into > >that > >state. > > It Also returns null for loops with multiple latches. So the patch looks OK for me. The bug I was seeing has been fixed by the patch for: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64284 Thanks, Sebastian
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c index 12f83ba..f8c736e 100644 --- a/gcc/tree-ssa-threadupdate.c +++ b/gcc/tree-ssa-threadupdate.c @@ -2564,6 +2564,7 @@ thread_through_all_blocks (bool may_peel_loop_headers) FOR_EACH_LOOP (loop, LI_FROM_INNERMOST) { if (!loop->header + || !loop_latch_edge (loop) || !bitmap_bit_p (threaded_blocks, loop->header->index)) continue;