| Message ID | 20160108201728.GF18720@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/08/2016 09:17 PM, Jakub Jelinek wrote: > As mentioned in the PR, sched1 and reload add NOTE_INSN_DELETED notes > that are moved by shrink-wrapping in between some basic blocks and > later on we end up with a barrier after the notes. From comments above > cleanup_barriers pass I think it isnot invalid, and various other places > deal with notes before barrier, so this patch teaches rtl_merge_block > to deal with that too. Hmm, there is something about this that bothers me. If the block did not end in a jump, then why is there a barrier? (insn 97 20 90 4 (cond_exec (eq (reg:CC 100 cc) (const_int 0 [0])) (set (mem/f:SI (plus:SI (reg/f:SI 0 r0 [orig:114 this ] [114]) (const_int 4 [0x4])) [2 this_5(D)->f+0 S4 A32]) (reg/f:SI 2 r2 [orig:110 b.0_4 ] [110]))) 69175.c:25 3729 {*p *thumb2_movsi_vfp} (nil)) (note 90 97 91 NOTE_INSN_DELETED) (note 91 90 101 NOTE_INSN_DELETED) (barrier 101 91 23) This seems like invalid incoming RTL - I don't mind the notes before the barrier, but I do mind the absence of a jump. If the jump was removed earlier, the barrier should have been removed at that point too IMO. Bernd
On Fri, Jan 08, 2016 at 10:09:37PM +0100, Bernd Schmidt wrote: > On 01/08/2016 09:17 PM, Jakub Jelinek wrote: > >As mentioned in the PR, sched1 and reload add NOTE_INSN_DELETED notes > >that are moved by shrink-wrapping in between some basic blocks and > >later on we end up with a barrier after the notes. From comments above > >cleanup_barriers pass I think it isnot invalid, and various other places > >deal with notes before barrier, so this patch teaches rtl_merge_block > >to deal with that too. > > Hmm, there is something about this that bothers me. If the block did not end > in a jump, then why is there a barrier? There is a jump, the removal of it is part of the conditional_execution ifcvt. The exact sequence of the relevant changes is: 1) sched1 adds (note 91 52 0 NOTE_INSN_DELETED) at the end of the IL 2) reload turns that into: (note 91 52 92 NOTE_INSN_DELETED) (note 92 91 0 NOTE_INSN_DELETED) 3) shrink-wrapping moves that around: (jump_insn 122 124 123 11 (simple_return) pr69175.C:26 -1 (nil) -> simple_return) ;; succ: EXIT [100.0%] ;; lr out ;; live out (barrier 123 122 91) (note 91 123 92 NOTE_INSN_DELETED) (note 92 91 109 NOTE_INSN_DELETED) ;; basic block 12, loop depth 0, count 0, freq 137, maybe hot 4) compgotos moves things around even further: ... (jump_insn 118 99 91 5 (simple_return) -1 (nil) -> simple_return) ;; succ: EXIT [100.0%] ;; lr out 0 [r0] 2 [r2] 4 [r4] 13 [sp] 14 [lr] ;; live out 0 [r0] 2 [r2] 4 [r4] 13 [sp] 14 [lr] (note 91 118 92 NOTE_INSN_DELETED) (note 92 91 102 NOTE_INSN_DELETED) (barrier 102 92 24) ;; basic block 6, loop depth 0, count 0, freq 1146, maybe hot ... 5) and that is how it survives until ce3, where it sees a test_bb (4) with 2 successors (then_bb 5 and else_bb 6), both of which simple_return and thus have a single successor - EXIT. Now, the merge_if_block caller removes the simple_return from the then_bb in preparation of the merge and expects the two bbs to be merged together, where the NOTE_INSN_DELETED of course can stay (cause no harm), but a barrier in the middle of bb is of course harmful. So, the question is, is what 4) - compgotos ended up with above already invalid RTL or not? I'm not sure about, but just used the: /* Some old code expects exactly one BARRIER as the NEXT_INSN of a non-fallthru insn. This is not generally true, as multiple barriers may have crept in, or the BARRIER may be separated from the last real insn by one or more NOTEs. This simple pass moves barriers and removes duplicates so that the old code is happy. */ comment as a hint that it might be ok, and just old backend code in machine reorgs etc. (after cleanup_barriers pass) might not be prepared for that. After all, even NOTE_INSN_CALL_ARG_LOCATION should come in between noreturn call and before corresponding barrier. Jakub
On 01/08/2016 02:23 PM, Jakub Jelinek wrote: > 4) compgotos moves things around even further: > ... > (jump_insn 118 99 91 5 (simple_return) -1 > (nil) > -> simple_return) > ;; succ: EXIT [100.0%] > ;; lr out 0 [r0] 2 [r2] 4 [r4] 13 [sp] 14 [lr] > ;; live out 0 [r0] 2 [r2] 4 [r4] 13 [sp] 14 [lr] > > (note 91 118 92 NOTE_INSN_DELETED) > (note 92 91 102 NOTE_INSN_DELETED) > (barrier 102 92 24) > ;; basic block 6, loop depth 0, count 0, freq 1146, maybe hot I think we're still OK at this point. Code that assumes the barrier immediately follows the jump has always been a bit iffy. Certainly it was the case back in 2001 when Jan/Andreas J. added this code that they were seeing cases where code made those assumptions, but they were not true/valid assumptions. It may be the case that the "old code" referred to by that comment is no longer around, but in the absence of an official "there must be nothing between the jump & any associated barrier", the safe thing to do is assume there may be notes and multiple barriers and handle them correctly. jeff
On 01/08/2016 10:23 PM, Jakub Jelinek wrote: > Now, the merge_if_block caller removes the simple_return from the then_bb > in preparation of the merge and expects the two bbs to be merged together, I'd be tempted to remove the barrier as well at this point. It does look like the cfgrtl code tries to handle isolated barriers, but IMO it's poor practice given that it's easy to avoid here. Bernd
On 01/08/2016 01:17 PM, Jakub Jelinek wrote: > Hi! > > As mentioned in the PR, sched1 and reload add NOTE_INSN_DELETED notes > that are moved by shrink-wrapping in between some basic blocks and > later on we end up with a barrier after the notes. From comments above > cleanup_barriers pass I think it isnot invalid, and various other places > deal with notes before barrier, so this patch teaches rtl_merge_block > to deal with that too. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > Independently, it would be nice if sched and reload clean those > NOTE_INSN_DELETED after themselves, there should be no reason why those > should be hanging around until final. > > 2016-01-08 Jakub Jelinek <jakub@redhat.com> > > PR target/69175 > * cfgrtl.c (rtl_merge_blocks): Look for BARRIER even after > notes outside of basic block. > > * g++.dg/opt/pr69175.C: New test. And presumably with this change, END includes the BARRIER and thus the BARRIER gets deleted, right? I know Bernd is engaged, so I'll let him have the final say here. jeff
On 01/08/2016 11:05 PM, Bernd Schmidt wrote: > On 01/08/2016 10:23 PM, Jakub Jelinek wrote: >> Now, the merge_if_block caller removes the simple_return from the then_bb >> in preparation of the merge and expects the two bbs to be merged >> together, > > I'd be tempted to remove the barrier as well at this point. It does look > like the cfgrtl code tries to handle isolated barriers, but IMO it's > poor practice given that it's easy to avoid here. Well, I checked a bit more. Most callers of merge_blocks seem to already look for barriers if they are a concern and remove them. This occurs multiple times in ifcvt.c and cfgcleanup.c. Oddly, merge_blocks_move_predecessor_nojumps uses next_nonnote_insn to find the barrier, while merge_blocks_move_successor_nojumps uses just NEXT_INSN. That should probably be fixed too. So the situation is a bit odd in that most callers remove the barrier but merge_blocks tries to handle an isolated barrier as well. The area could probably cleaned up a little, but on the whole I still lean towards requiring the caller to remove an isolated barrier. That leaves the RTL in a more consistent state before the call to merge_blocks. Bernd
--- gcc/cfgrtl.c.jj 2016-01-04 14:55:50.000000000 +0100 +++ gcc/cfgrtl.c 2016-01-08 12:57:00.973745865 +0100 @@ -875,8 +875,15 @@ rtl_merge_blocks (basic_block a, basic_b a_end = PREV_INSN (del_first); } - else if (BARRIER_P (NEXT_INSN (a_end))) - del_first = NEXT_INSN (a_end); + else + { + rtx_insn *end = NEXT_INSN (a_end); + while (end && NOTE_P (end) && !NOTE_INSN_BASIC_BLOCK_P (end)) + end = NEXT_INSN (end); + + if (end && BARRIER_P (end)) + del_first = end; + } /* Delete everything marked above as well as crap that might be hanging out between the two blocks. */ --- gcc/testsuite/g++.dg/opt/pr69175.C.jj 2016-01-08 13:04:04.084805432 +0100 +++ gcc/testsuite/g++.dg/opt/pr69175.C 2016-01-08 13:03:47.000000000 +0100 @@ -0,0 +1,29 @@ +// PR target/69175 +// { dg-do compile } +// { dg-options "-O2" } +// { dg-additional-options "-march=armv7-a -mfloat-abi=hard -mfpu=vfpv3-d16 -mthumb" { target { arm_hard_vfp_ok && arm_thumb2_ok } } } + +struct A { A *c, *d; } a; +struct B { A *e; A *f; void foo (); }; +void *b; + +void +B::foo () +{ + if (b) + { + A *n = (A *) b; + if (b == e) + if (n == f) + e = __null; + else + e->c = __null; + else + n->d->c = &a; + n->d = e; + if (e == __null) + e = f = n; + else + e = n; + } +}