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
