Message ID | 56547D49.5040609@mentor.com |
---|---|
State | New |
Headers | show |
On Tue, 24 Nov 2015, Tom de Vries wrote: > [ was: Re: [PATCH] Check NULL loop->latch in verify_loop_structure ] > > On 23/11/15 11:28, Richard Biener wrote: > > On Mon, 23 Nov 2015, Tom de Vries wrote: > > > > > Hi, > > > > > > In verify_loop_structure, we stop checking the latch once we find that > > > it's > > > NULL. > > > > > > This patch tries a bit harder: > > > - if !LOOPS_MAY_HAVE_MULTIPLE_LATCHES, we don't allow a NULL latch > > > - if LOOPS_MAY_HAVE_MULTIPLE_LATCHES, we check that indeed there's no > > > single loop latch. > > > > > > As a consequence of adding this check, I needed to fix > > > expand_omp_for_generic, > > > which missed an initialization of a loop latch. > > > > > > Bootstrapped and reg-tested on x86_64. > > > > > > OK for stage3 trunk? > > > > You miss to catch the case where loop->latch is non-NULL but there > > are multiple latches, so I think the patch can be improved. > > That case is more important for correctness (passes > > seeing ->latch non-NULL assume a single latch). > > > > Updated according to comment. > > Bootstrapped and reg-tested on x86_64. > > OK for stage3 trunk? Ok. Thanks, Richard.
This caused an ICE compiling value.c from gdb on aarch64-none-linux-gnu; the testcase, after preprocessing on aarch64, ICEs on both aarch64 and x86_64, but is about 1MB - I'm working on reducing that down to something small enough to post... $ ./gcc/xgcc -B ./gcc -O2 -g value.c ../../binutils-gdb/gdb/value.c: In function ‘show_convenience’: ../../binutils-gdb/gdb/value.c:2615:1: error: loop 3’s latch is missing ../../binutils-gdb/gdb/value.c:2615:1: internal compiler error: in verify_loop_structure, at cfgloop.c:1669 0x71e653 verify_loop_structure() /work/alalaw01/src2/gcc/gcc/cfgloop.c:1669 0x97c6ae checking_verify_loop_structure /work/alalaw01/src2/gcc/gcc/cfgloop.h:325 0x97c6ae loop_optimizer_init(unsigned int) /work/alalaw01/src2/gcc/gcc/loop-init.c:106 0x97c78a rtl_loop_init /work/alalaw01/src2/gcc/gcc/loop-init.c:398 0x97c78a execute /work/alalaw01/src2/gcc/gcc/loop-init.c:425 --Alan
Here's a reduced testcase (reduced to the point of generating lots of warnings, I'm compiling with -O2 -w, on x86_64): struct __jmp_buf_tag { }; typedef struct __jmp_buf_tag sigjmp_buf[1]; extern struct cmd_list_element *showlist; struct internalvar { struct internalvar *next; }; static struct internalvar *internalvars; struct internalvar * create_internalvar (const char *name) { struct internalvar *var = ((struct internalvar *) xmalloc (sizeof (struct internalvar))); internalvars = var; } void show_convenience (char *ignore, int from_tty) { struct gdbarch *gdbarch = get_current_arch (); int varseen = 0; for (struct internalvar *var = internalvars; var; var = var->next) { if (!varseen) varseen = 1; sigjmp_buf *buf = exceptions_state_mc_init (); __sigsetjmp (); while (exceptions_state_mc_action_iter ()) while (exceptions_state_mc_action_iter_1 ()) ; } if (!varseen) printf_unfiltered (); } On 26 November 2015 at 11:33, Alan Lawrence <alan.lawrence@arm.com> wrote: > This caused an ICE compiling value.c from gdb on > aarch64-none-linux-gnu; the testcase, after preprocessing on aarch64, > ICEs on both aarch64 and x86_64, but is about 1MB - I'm working on > reducing that down to something small enough to post... > > $ ./gcc/xgcc -B ./gcc -O2 -g value.c > ../../binutils-gdb/gdb/value.c: In function ‘show_convenience’: > ../../binutils-gdb/gdb/value.c:2615:1: error: loop 3’s latch is missing > ../../binutils-gdb/gdb/value.c:2615:1: internal compiler error: in > verify_loop_structure, at cfgloop.c:1669 > 0x71e653 verify_loop_structure() > /work/alalaw01/src2/gcc/gcc/cfgloop.c:1669 > 0x97c6ae checking_verify_loop_structure > /work/alalaw01/src2/gcc/gcc/cfgloop.h:325 > 0x97c6ae loop_optimizer_init(unsigned int) > /work/alalaw01/src2/gcc/gcc/loop-init.c:106 > 0x97c78a rtl_loop_init > /work/alalaw01/src2/gcc/gcc/loop-init.c:398 > 0x97c78a execute > /work/alalaw01/src2/gcc/gcc/loop-init.c:425 > > --Alan
On Thu, 26 Nov 2015, Alan Lawrence wrote: > This caused an ICE compiling value.c from gdb on > aarch64-none-linux-gnu; the testcase, after preprocessing on aarch64, > ICEs on both aarch64 and x86_64, but is about 1MB - I'm working on > reducing that down to something small enough to post... > > $ ./gcc/xgcc -B ./gcc -O2 -g value.c > ../../binutils-gdb/gdb/value.c: In function ‘show_convenience’: > ../../binutils-gdb/gdb/value.c:2615:1: error: loop 3’s latch is missing > ../../binutils-gdb/gdb/value.c:2615:1: internal compiler error: in > verify_loop_structure, at cfgloop.c:1669 > 0x71e653 verify_loop_structure() > /work/alalaw01/src2/gcc/gcc/cfgloop.c:1669 > 0x97c6ae checking_verify_loop_structure > /work/alalaw01/src2/gcc/gcc/cfgloop.h:325 > 0x97c6ae loop_optimizer_init(unsigned int) > /work/alalaw01/src2/gcc/gcc/loop-init.c:106 > 0x97c78a rtl_loop_init > /work/alalaw01/src2/gcc/gcc/loop-init.c:398 > 0x97c78a execute > /work/alalaw01/src2/gcc/gcc/loop-init.c:425 See also PR68549 for why I think this happens "by design". Thus I think we need to revert the checking when LOOPS_MAY_HAVE_MULTIPLE_LATCHES for now. Richard.
On 26/11/15 13:15, Richard Biener wrote: > On Thu, 26 Nov 2015, Alan Lawrence wrote: > >> This caused an ICE compiling value.c from gdb on >> aarch64-none-linux-gnu; the testcase, after preprocessing on aarch64, >> ICEs on both aarch64 and x86_64, but is about 1MB - I'm working on >> reducing that down to something small enough to post... >> >> $ ./gcc/xgcc -B ./gcc -O2 -g value.c >> ../../binutils-gdb/gdb/value.c: In function ‘show_convenience’: >> ../../binutils-gdb/gdb/value.c:2615:1: error: loop 3’s latch is missing >> ../../binutils-gdb/gdb/value.c:2615:1: internal compiler error: in >> verify_loop_structure, at cfgloop.c:1669 >> 0x71e653 verify_loop_structure() >> /work/alalaw01/src2/gcc/gcc/cfgloop.c:1669 >> 0x97c6ae checking_verify_loop_structure >> /work/alalaw01/src2/gcc/gcc/cfgloop.h:325 >> 0x97c6ae loop_optimizer_init(unsigned int) >> /work/alalaw01/src2/gcc/gcc/loop-init.c:106 >> 0x97c78a rtl_loop_init >> /work/alalaw01/src2/gcc/gcc/loop-init.c:398 >> 0x97c78a execute >> /work/alalaw01/src2/gcc/gcc/loop-init.c:425 > > See also PR68549 for why I think this happens "by design". Thus > I think we need to revert the checking when > LOOPS_MAY_HAVE_MULTIPLE_LATCHES for now. I'll revert the whole patch for now. Thanks, - Tom
Improve verification of loop->latch in verify_loop_structure 2015-11-22 Tom de Vries <tom@codesourcery.com> * cfgloop.c (find_single_latch): New function, factored out of ... (flow_loops_find): ... here. (verify_loop_structure): Improve verification of loop->latch. * cfgloop.h (find_single_latch): Declare. * omp-low.c (expand_omp_for_generic): Initialize latch of orig_loop. --- gcc/cfgloop.c | 70 ++++++++++++++++++++++++++++++++++++++++++----------------- gcc/cfgloop.h | 1 + gcc/omp-low.c | 1 + 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/gcc/cfgloop.c b/gcc/cfgloop.c index bf00e0e..a6850e3 100644 --- a/gcc/cfgloop.c +++ b/gcc/cfgloop.c @@ -388,6 +388,33 @@ bb_loop_header_p (basic_block header) return false; } +/* Return the latch block for this header block, if it has just a single one. + Otherwise, return NULL. */ + +basic_block +find_single_latch (struct loop* loop) +{ + basic_block header = loop->header; + edge_iterator ei; + edge e; + basic_block latch = NULL; + + FOR_EACH_EDGE (e, ei, header->preds) + { + basic_block cand = e->src; + if (!flow_bb_inside_loop_p (loop, cand)) + continue; + + if (latch != NULL) + /* More than one latch edge. */ + return NULL; + + latch = cand; + } + + return latch; +} + /* Find all the natural loops in the function and save in LOOPS structure and recalculate loop_father information in basic block structures. If LOOPS is non-NULL then the loop structures for already recorded loops @@ -482,29 +509,10 @@ flow_loops_find (struct loops *loops) { struct loop *loop = larray[i]; basic_block header = loop->header; - edge_iterator ei; - edge e; flow_loop_tree_node_add (header->loop_father, loop); loop->num_nodes = flow_loop_nodes_find (loop->header, loop); - - /* Look for the latch for this header block, if it has just a - single one. */ - FOR_EACH_EDGE (e, ei, header->preds) - { - basic_block latch = e->src; - - if (flow_bb_inside_loop_p (loop, latch)) - { - if (loop->latch != NULL) - { - /* More than one latch edge. */ - loop->latch = NULL; - break; - } - loop->latch = latch; - } - } + loop->latch = find_single_latch (loop); } return loops; @@ -1439,6 +1447,28 @@ verify_loop_structure (void) error ("loop %d%'s latch is not dominated by its header", i); err = 1; } + if (find_single_latch (loop) == NULL) + { + error ("loop %d%'s latch is is not the only latch", i); + err = 1; + } + } + else + { + if (loops_state_satisfies_p (LOOPS_MAY_HAVE_MULTIPLE_LATCHES)) + { + if (find_single_latch (loop) != NULL) + { + error ("loop %d%'s latch is missing", i); + err = 1; + } + } + else + { + error ("loop %d%'s latch is missing, and loops may not have" + " multiple latches", i); + err = 1; + } } if (loops_state_satisfies_p (LOOPS_HAVE_SIMPLE_LATCHES)) { diff --git a/gcc/cfgloop.h b/gcc/cfgloop.h index ee73bf9..7faf591 100644 --- a/gcc/cfgloop.h +++ b/gcc/cfgloop.h @@ -270,6 +270,7 @@ bool mark_irreducible_loops (void); void release_recorded_exits (function *); void record_loop_exits (void); void rescan_loop_exit (edge, bool, bool); +basic_block find_single_latch (struct loop*); /* Loop data structure manipulation/querying. */ extern void flow_loop_tree_node_add (struct loop *, struct loop *); diff --git a/gcc/omp-low.c b/gcc/omp-low.c index 0d4c6e5..2d782eb 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -8903,6 +8903,7 @@ expand_omp_for_generic (struct omp_region *region, orig_loop->header = l1_bb; /* The loop may have multiple latches. */ add_loop (orig_loop, new_loop); + orig_loop->latch = find_single_latch (orig_loop); } } }