Re: [patch] PR middle-end/43631
This didn't really help make things look prettier. The notes are
enumerated in many other places, for various purposes. I didn't like
special-casing this one INSN_NOTE oddity when there are so many others
already. So I created a note_outside_basic_block_p instead, I hope you
agree with this approach.
Sure, thanks for investigating the proposal in any case.
Updated patch attached, similarly tested. OK for trunk?
Yes, modulo a few nits:
+/* Like add_insn_after, but try to set BLOCK_FOR_INSN.
+ If BB is NULL, an attempt is made to infer the bb from before.
+
+ This and the next function should be the only functions called
+ to insert an insn once delay slots have been filled since only
+ they know how to update a SEQUENCE. */
+
+void
+add_insn_after (rtx insn, rtx after, basic_block bb)
Like add_insn_after_nobb, ...
-/* Add INSN into the doubly-linked list before insn BEFORE. This and
- the previous should be the only functions called to insert an insn
- once delay slots have been filled since only they know how to
- update a SEQUENCE. If BB is NULL, an attempt is made to infer the
- bb from before. */
+/* Like add_insn_before_nobb, but try to set BLOCK_FOR_INSN.
+ If BB is NULL, an attempt is made to infer the bb from before. */
void
add_insn_before (rtx insn, rtx before, basic_block bb)
Keep the blurb about SEQUENCEs: This and the previous function should...
+/* Return true iff a note of kind SUBTYPE should be emitted with via
+ routines that never set BLOCK_FOR_INSN on NOTE. BB_BOUNDARY is true
+ if the caller is asked to emit a note before BB_HEAD, or after BB_END. */
Superfluous with or via.
+static bool
+note_outside_basic_block_p (enum insn_note subtype, bool on_bb_boundary_p)
+{
This should be implemented with a switch statement.
--
Eric Botcazou
Re: [patch] PR middle-end/43631
On Fri, Apr 12, 2013 at 6:46 PM, Steven Bosscher wrote:
Can we reorganize insn-notes.def so that the 3 classes are clearly separated
(and optionally define a NOTE_INSN_CLASS macro so that we don't need to
enumerate the notes each time)?
That's probably a good idea, yes.
This didn't really help make things look prettier. The notes are
enumerated in many other places, for various purposes. I didn't like
special-casing this one INSN_NOTE oddity when there are so many others
already. So I created a note_outside_basic_block_p instead, I hope you
agree with this approach.
While there, I noticed that we could use a make_note_raw.
Reading your patch, it apppears that NOTE_INSN_SWITCH_TEXT_SECTIONS is the
only member of the 1st class and that almost all notes are in the 2nd class
except for the 2 LOCATION and the 2 EH_REGION recently added notes.
I think DELETED_LABEL and DELETED_DEBUG_LABEL also can only live
outside basic blocks (i.e. like SWITCH_TEXT_SECTIONS), but I'm not
sure.
It turns out that DELETED_LABEL cannot be emitted, it can only be
created by patching out a label. Likewise for DELETED_DEBUG_LABEL, so
I've included that one also in the gcc_assert for this.
- Remove the
This and the next should be the only functions called to insert an insn
once delay slots have been filled since only they know how to update a
SEQUENCE.
from the head comment of functions that are now private to emit-rtl.c.
OK.
Done.
Updated patch attached, similarly tested. OK for trunk?
Ciao!
Steven
PR middle-end/43631
* emit-rtl.c (make_note_raw): New function.
(link_insn_into_chain): New static inline function.
(add_insn): Use it.
(add_insn_before, add_insn_after): Factor insn chain linking code...
(add_insn_before_nobb, add_insn_after_nobb): ...here, new functions
using link_insn_into_chain.
(note_outside_basic_block_p): New helper function for emit_note_after
and emit_note_before.
(emit_note_after): Use nobb variant of add_insn_after if the note
should not be contained in a basic block.
(emit_note_before): Use nobb variant of add_insn_before if the note
should not be contained in a basic block.
(emit_note_copy): Use make_note_raw.
(emit_note): Likewise.
* bb-reorder.c (insert_section_boundary_note): Remove hack to set
BLOCK_FOR_INSN to NULL manually for NOTE_INSN_SWITCH_TEXT_SECTIONS.
* jump.c (cleanup_barriers): Use reorder_insns_nobb to avoid making
the moved barrier the tail of the basic block it follows.
* var-tracking.c (pass_variable_tracking): Add TODO_verify_flow.
Index: emit-rtl.c
===
--- emit-rtl.c (revision 197942)
+++ emit-rtl.c (working copy)
@@ -3751,62 +3751,135 @@ make_call_insn_raw (rtx pattern)
return insn;
}
+
+/* Like `make_insn_raw' but make a NOTE instead of an insn. */
+
+static rtx
+make_note_raw (enum insn_note subtype)
+{
+ /* Some notes are never created this way at all. These notes are
+ only created by patching out insns. */
+ gcc_assert (subtype != NOTE_INSN_DELETED_LABEL
+ subtype != NOTE_INSN_DELETED_DEBUG_LABEL);
+
+ rtx note = rtx_alloc (NOTE);
+ INSN_UID (note) = cur_insn_uid++;
+ NOTE_KIND (note) = subtype;
+ BLOCK_FOR_INSN (note) = NULL;
+ memset (NOTE_DATA (note), 0, sizeof (NOTE_DATA (note)));
+ return note;
+}
+/* Add INSN to the end of the doubly-linked list, between PREV and NEXT.
+ INSN may be any object that can appear in the chain: INSN_P and NOTE_P
objects,
+ but also BARRIERs and JUMP_TABLE_DATAs. PREV and NEXT may be NULL. */
+
+static inline void
+link_insn_into_chain (rtx insn, rtx prev, rtx next)
+{
+ PREV_INSN (insn) = prev;
+ NEXT_INSN (insn) = next;
+ if (prev != NULL)
+{
+ NEXT_INSN (prev) = insn;
+ if (NONJUMP_INSN_P (prev) GET_CODE (PATTERN (prev)) == SEQUENCE)
+ {
+ rtx sequence = PATTERN (prev);
+ NEXT_INSN (XVECEXP (sequence, 0, XVECLEN (sequence, 0) - 1)) = insn;
+ }
+}
+ if (next != NULL)
+{
+ PREV_INSN (next) = insn;
+ if (NONJUMP_INSN_P (next) GET_CODE (PATTERN (next)) == SEQUENCE)
+ PREV_INSN (XVECEXP (PATTERN (next), 0, 0)) = insn;
+}
+}
+
/* Add INSN to the end of the doubly-linked list.
INSN may be an INSN, JUMP_INSN, CALL_INSN, CODE_LABEL, BARRIER or NOTE. */
void
add_insn (rtx insn)
{
- PREV_INSN (insn) = get_last_insn();
- NEXT_INSN (insn) = 0;
-
- if (NULL != get_last_insn())
-NEXT_INSN (get_last_insn ()) = insn;
-
+ rtx prev = get_last_insn ();
+ link_insn_into_chain (insn, prev, NULL);
if (NULL == get_insns ())
set_first_insn (insn);
-
set_last_insn (insn);
}
-/* Add INSN into the doubly-linked list after insn AFTER. This and
- the next should be the only functions called to insert an insn once
- delay slots have been filled since only
Re: [patch] PR middle-end/43631
The problem is actually bigger than just the var-tracking notes. The general problem is that there are no rules for whether or not notes are part of a basic block or not. Some notes never appear inside a basic block, some notes always must appear inside a basic block, and some can appear virtually anywhere. Can we reorganize insn-notes.def so that the 3 classes are clearly separated (and optionally define a NOTE_INSN_CLASS macro so that we don't need to enumerate the notes each time)? Reading your patch, it apppears that NOTE_INSN_SWITCH_TEXT_SECTIONS is the only member of the 1st class and that almost all notes are in the 2nd class except for the 2 LOCATION and the 2 EH_REGION recently added notes. With this patch I've chosen to maintain the invariant that BB_END must be an INSN, and never be a NOTE or BARRIER. Yes, I think that's the historical design. The result is that NOTE_INSN_EH_REGION_BEG notes can be inside a basic block while the pairing NOTE_INSN_EH_REGION_END is outside the basic block. I don't think this is a problem: The same thing already happens with jump table data, barriers, etc. Slightly annoying, but trumped by the previous requirement I think. The nice thing is that with this patch, *finally* I have verify_flow_info pass after var-tracking. That's important because officially the CFG is freed after this pass (pass_free_cfg runs after pass_variable_tracking) but because verify_flow_info didn't pass after var-tracking the CFG was already invalidated before it was freed (BB_HEAD and BB_END would be incorrect). That has the effect that some of the machine-reorg passes that use the CFG never really had a correct CFG at all! Thanks for working on this. Some nits in the patch: - Remove the This and the next should be the only functions called to insert an insn once delay slots have been filled since only they know how to update a SEQUENCE. from the head comment of functions that are now private to emit-rtl.c. - Are there still public functions that cannot be invoked in emit-rtl.c to insert insns where there are SEQUENCEs in the stream? If so, you need to mark the ones which can be invoked explicitly with a blurb in the head comment. -- Eric Botcazou
Re: [patch] PR middle-end/43631
On Fri, Apr 12, 2013 at 9:48 AM, Eric Botcazou ebotca...@adacore.com wrote: The problem is actually bigger than just the var-tracking notes. The general problem is that there are no rules for whether or not notes are part of a basic block or not. Some notes never appear inside a basic block, some notes always must appear inside a basic block, and some can appear virtually anywhere. Can we reorganize insn-notes.def so that the 3 classes are clearly separated (and optionally define a NOTE_INSN_CLASS macro so that we don't need to enumerate the notes each time)? That's probably a good idea, yes. I can sort the notes in insn-notes.def, add a field to DEF_INSN_NOTE, and add a NOTE_INSN_CLASS to rtl.h. I'll update the patch and re-submit. Reading your patch, it apppears that NOTE_INSN_SWITCH_TEXT_SECTIONS is the only member of the 1st class and that almost all notes are in the 2nd class except for the 2 LOCATION and the 2 EH_REGION recently added notes. I think DELETED_LABEL and DELETED_DEBUG_LABEL also can only live outside basic blocks (i.e. like SWITCH_TEXT_SECTIONS), but I'm not sure. - Remove the This and the next should be the only functions called to insert an insn once delay slots have been filled since only they know how to update a SEQUENCE. from the head comment of functions that are now private to emit-rtl.c. OK. - Are there still public functions that cannot be invoked in emit-rtl.c to insert insns where there are SEQUENCEs in the stream? If so, you need to mark the ones which can be invoked explicitly with a blurb in the head comment. Well, only add_insn if you don't count the emit_* functions. add_insn already has a comment that says what it can handle. I will explicitly add a remark to it that it doesn't handle SEQUENCE. NB: I didn't really change anything here, I only wanted add_insn_before and add_insn_after to share more code because I have follow-up patches to make add_insn_before/add_insn_after capable of inserting a SEQUENCE insn, too (reorg.c now manually splices them into the insn chain). I'll send an updated patch this weekend. Ciao! Steven
Re: [patch] PR middle-end/43631
Premature ping? Also bootstrappedtested on ia64-unknown-linux-gnu now. On Sun, Apr 7, 2013 at 12:40 AM, Steven Bosscher wrote: Hello, In this PR43631, var-tracking notes are inserted on basic block boundaries and BB_HEAD/BB_END end up pointing to these notes. This breaks verify_flow_info. The problem is actually bigger than just the var-tracking notes. The general problem is that there are no rules for whether or not notes are part of a basic block or not. Some notes never appear inside a basic block, some notes always must appear inside a basic block, and some can appear virtually anywhere. With this patch I've chosen to maintain the invariant that BB_END must be an INSN, and never be a NOTE or BARRIER. The result is that NOTE_INSN_EH_REGION_BEG notes can be inside a basic block while the pairing NOTE_INSN_EH_REGION_END is outside the basic block. I don't think this is a problem: The same thing already happens with jump table data, barriers, etc. The nice thing is that with this patch, *finally* I have verify_flow_info pass after var-tracking. That's important because officially the CFG is freed after this pass (pass_free_cfg runs after pass_variable_tracking) but because verify_flow_info didn't pass after var-tracking the CFG was already invalidated before it was freed (BB_HEAD and BB_END would be incorrect). That has the effect that some of the machine-reorg passes that use the CFG never really had a correct CFG at all! With this patch and a hack to call compute_bb_for_insn, I now can call verify_flow_info in every pass up to final for the i386 port :-) Bootstrappedtested on x86_64-unknown-linux-gnu and powerpc64-unknown-linux-gnu. OK for trunk? Ciao! Steven
[patch] PR middle-end/43631
Hello,
In this PR43631, var-tracking notes are inserted on basic block
boundaries and BB_HEAD/BB_END end up pointing to these notes. This
breaks verify_flow_info.
The problem is actually bigger than just the var-tracking notes. The
general problem is that there are no rules for whether or not notes
are part of a basic block or not. Some notes never appear inside a
basic block, some notes always must appear inside a basic block, and
some can appear virtually anywhere.
With this patch I've chosen to maintain the invariant that BB_END must
be an INSN, and never be a NOTE or BARRIER. The result is that
NOTE_INSN_EH_REGION_BEG notes can be inside a basic block while the
pairing NOTE_INSN_EH_REGION_END is outside the basic block. I don't
think this is a problem: The same thing already happens with jump
table data, barriers, etc.
The nice thing is that with this patch, *finally* I have
verify_flow_info pass after var-tracking. That's important because
officially the CFG is freed after this pass (pass_free_cfg runs after
pass_variable_tracking) but because verify_flow_info didn't pass after
var-tracking the CFG was already invalidated before it was freed
(BB_HEAD and BB_END would be incorrect). That has the effect that some
of the machine-reorg passes that use the CFG never really had a
correct CFG at all!
With this patch and a hack to call compute_bb_for_insn, I now can call
verify_flow_info in every pass up to final for the i386 port :-)
Bootstrappedtested on x86_64-unknown-linux-gnu and powerpc64-unknown-linux-gnu.
OK for trunk?
Ciao!
Steven
PR middle-end/43631
* emit-rtl.c (link_insn_into_chain): New static inline function.
(add_insn): Use it.
(add_insn_before, add_insn_after): Factor insn chain linking code...
(add_insn_before_nobb, add_insn_after_nobb): ...here, new functions
using link_insn_into_chain.
(emit_note_after): Use nobb variant of add_insn_after if the note
should not be contained in a basic block.
(emit_note_before): Use nobb variant of add_insn_before if the note
should not be contained in a basic block.
* bb-reorder.c (insert_section_boundary_note): Remove hack to set
BLOCK_FOR_INSN to NULL manually for NOTE_INSN_SWITCH_TEXT_SECTIONS.
* jump.c (cleanup_barriers): Use reorder_insns_nobb to avoid making
the moved barrier the tail of the basic block it follows.
* var-tracking.c (pass_variable_tracking): Add TODO_verify_flow.
Index: emit-rtl.c
===
--- emit-rtl.c (revision 197536)
+++ emit-rtl.c (working copy)
@@ -3752,61 +3752,122 @@ make_call_insn_raw (rtx pattern)
return insn;
}
+/* Add INSN to the end of the doubly-linked list, between PREV and NEXT.
+ INSN may be any object that can appear in the chain: INSN_P and NOTE_P
objects,
+ but also BARRIERs and JUMP_TABLE_DATAs. PREV and NEXT may be NULL. */
+
+static inline void
+link_insn_into_chain (rtx insn, rtx prev, rtx next)
+{
+ PREV_INSN (insn) = prev;
+ NEXT_INSN (insn) = next;
+ if (prev != NULL)
+{
+ NEXT_INSN (prev) = insn;
+ if (NONJUMP_INSN_P (prev) GET_CODE (PATTERN (prev)) == SEQUENCE)
+ {
+ rtx sequence = PATTERN (prev);
+ NEXT_INSN (XVECEXP (sequence, 0, XVECLEN (sequence, 0) - 1)) = insn;
+ }
+}
+ if (next != NULL)
+{
+ PREV_INSN (next) = insn;
+ if (NONJUMP_INSN_P (next) GET_CODE (PATTERN (next)) == SEQUENCE)
+ PREV_INSN (XVECEXP (PATTERN (next), 0, 0)) = insn;
+}
+}
+
/* Add INSN to the end of the doubly-linked list.
INSN may be an INSN, JUMP_INSN, CALL_INSN, CODE_LABEL, BARRIER or NOTE. */
void
add_insn (rtx insn)
{
- PREV_INSN (insn) = get_last_insn();
- NEXT_INSN (insn) = 0;
-
- if (NULL != get_last_insn())
-NEXT_INSN (get_last_insn ()) = insn;
-
+ rtx prev = get_last_insn ();
+ link_insn_into_chain (insn, prev, NULL);
if (NULL == get_insns ())
set_first_insn (insn);
-
set_last_insn (insn);
}
/* Add INSN into the doubly-linked list after insn AFTER. This and
the next should be the only functions called to insert an insn once
delay slots have been filled since only they know how to update a
- SEQUENCE. */
+ SEQUENCE.
+ This function is internal to emit-rtl.c. Use add_insn_after outside
+ this file. */
-void
-add_insn_after (rtx insn, rtx after, basic_block bb)
+static void
+add_insn_after_nobb (rtx insn, rtx after)
{
rtx next = NEXT_INSN (after);
gcc_assert (!optimize || !INSN_DELETED_P (after));
- NEXT_INSN (insn) = next;
- PREV_INSN (insn) = after;
+ link_insn_into_chain (insn, after, next);
- if (next)
+ if (next == NULL)
{
- PREV_INSN (next) = insn;
- if (NONJUMP_INSN_P (next) GET_CODE (PATTERN (next)) == SEQUENCE)
- PREV_INSN (XVECEXP (PATTERN (next), 0, 0)) = insn;
+ if (get_last_insn () == after)
+ set_last_insn (insn);
+ else
+
