Patchwork PR middle-end/43631

login
register
mail settings
Submitter Steven Bosscher
Date April 6, 2013, 10:40 p.m.
Message ID <CABu31nPmRrkRfqjbYq-kfqHgjVxiYuvG9wvB+Z3sw_CgPd+GQw@mail.gmail.com>
Download mbox | patch
Permalink /patch/234455/
State New
Headers show

Comments

Steven Bosscher - April 6, 2013, 10:40 p.m.
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 :-)

Bootstrapped&tested 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.
Steven Bosscher - April 9, 2013, 4:53 p.m.
Premature ping?
Also bootstrapped&tested 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 :-)
>
> Bootstrapped&tested on x86_64-unknown-linux-gnu and powerpc64-unknown-linux-gnu.
> OK for trunk?
>
> Ciao!
> Steven
Eric Botcazou - April 12, 2013, 7:48 a.m.
> 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.
Steven Bosscher - April 12, 2013, 4:46 p.m.
On Fri, Apr 12, 2013 at 9:48 AM, Eric Botcazou <ebotcazou@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

Patch

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
+	{
+	  struct sequence_stack *stack = seq_stack;
+	  /* Scan all pending sequences too.  */
+	  for (; stack; stack = stack->next)
+	    if (after == stack->last)
+	      {
+		stack->last = insn;
+		break;
+	      }
+	}
     }
-  else if (get_last_insn () == after)
-    set_last_insn (insn);
-  else
+}
+
+/* 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.
+   This function is internal to emit-rtl.c.  Use add_insn_before outside
+   this file.  */
+
+static void
+add_insn_before_nobb (rtx insn, rtx before)
+{
+  rtx prev = PREV_INSN (before);
+
+  gcc_assert (!optimize || !INSN_DELETED_P (before));
+
+  link_insn_into_chain (insn, prev, before);
+
+  if (prev == NULL)
     {
-      struct sequence_stack *stack = seq_stack;
-      /* Scan all pending sequences too.  */
-      for (; stack; stack = stack->next)
-	if (after == stack->last)
-	  {
-	    stack->last = insn;
-	    break;
-	  }
+      if (get_insns () == before)
+	set_first_insn (insn);
+      else
+	{
+	  struct sequence_stack *stack = seq_stack;
+	  /* Scan all pending sequences too.  */
+	  for (; stack; stack = stack->next)
+	    if (before == stack->first)
+	      {
+		stack->first = insn;
+		break;
+	      }
 
-      gcc_assert (stack);
+	  gcc_assert (stack);
+	}
     }
+}
 
+/* 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.  */
+
+void
+add_insn_after (rtx insn, rtx after, basic_block bb)
+{
+  add_insn_after_nobb (insn, after);
   if (!BARRIER_P (after)
       && !BARRIER_P (insn)
       && (bb = BLOCK_FOR_INSN (after)))
@@ -3822,55 +3883,15 @@  add_insn_after (rtx insn, rtx after, bas
 	  && !NOTE_INSN_BASIC_BLOCK_P (insn))
 	BB_END (bb) = insn;
     }
-
-  NEXT_INSN (after) = insn;
-  if (NONJUMP_INSN_P (after) && GET_CODE (PATTERN (after)) == SEQUENCE)
-    {
-      rtx sequence = PATTERN (after);
-      NEXT_INSN (XVECEXP (sequence, 0, XVECLEN (sequence, 0) - 1)) = insn;
-    }
 }
 
-/* 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)
 {
-  rtx prev = PREV_INSN (before);
-
-  gcc_assert (!optimize || !INSN_DELETED_P (before));
-
-  PREV_INSN (insn) = prev;
-  NEXT_INSN (insn) = before;
-
-  if (prev)
-    {
-      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;
-	}
-    }
-  else if (get_insns () == before)
-    set_first_insn (insn);
-  else
-    {
-      struct sequence_stack *stack = seq_stack;
-      /* Scan all pending sequences too.  */
-      for (; stack; stack = stack->next)
-	if (before == stack->first)
-	  {
-	    stack->first = insn;
-	    break;
-	  }
-
-      gcc_assert (stack);
-    }
+  add_insn_before_nobb (insn, before);
 
   if (!bb
       && !BARRIER_P (before)
@@ -3889,10 +3910,6 @@  add_insn_before (rtx insn, rtx before, b
 		  || BARRIER_P (insn)
 		  || NOTE_INSN_BASIC_BLOCK_P (insn));
     }
-
-  PREV_INSN (before) = insn;
-  if (NONJUMP_INSN_P (before) && GET_CODE (PATTERN (before)) == SEQUENCE)
-    PREV_INSN (XVECEXP (PATTERN (before), 0, 0)) = insn;
 }
 
 
@@ -4230,21 +4247,6 @@  emit_label_before (rtx label, rtx before
   add_insn_before (label, before, NULL);
   return label;
 }
-
-/* Emit a note of subtype SUBTYPE before the insn BEFORE.  */
-
-rtx
-emit_note_before (enum insn_note subtype, rtx before)
-{
-  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)));
-
-  add_insn_before (note, before, NULL);
-  return note;
-}
 
 /* Helper for emit_insn_after, handles lists of instructions
    efficiently.  */
@@ -4391,18 +4393,73 @@  emit_label_after (rtx label, rtx after)
   add_insn_after (label, after, NULL);
   return label;
 }
+
+/* Notes require a bit of special handling: Some notes need to have their
+   BLOCK_FOR_INSN set, others should never have it set, and some should have
+   it set or clear depending on the context.   */
 
 /* Emit a note of subtype SUBTYPE after the insn AFTER.  */
 
 rtx
 emit_note_after (enum insn_note subtype, rtx after)
 {
+  /* Some notes are never emitted.  */
+  gcc_assert (subtype != NOTE_INSN_DELETED_LABEL);
+
+  basic_block bb = BARRIER_P (after) ? NULL : BLOCK_FOR_INSN (after);
   rtx note = rtx_alloc (NOTE);
   INSN_UID (note) = cur_insn_uid++;
   NOTE_KIND (note) = subtype;
   BLOCK_FOR_INSN (note) = NULL;
+  gcc_assert (subtype != NOTE_INSN_DELETED_LABEL);
   memset (&NOTE_DATA (note), 0, sizeof (NOTE_DATA (note)));
-  add_insn_after (note, after, NULL);
+
+  /* NOTE_INSN_SWITCH_TEXT_SECTIONS only appears between basic blocks.  */
+  if (subtype == NOTE_INSN_SWITCH_TEXT_SECTIONS)
+    add_insn_after_nobb (note, after);
+  /* Notes for var tracking and EH region markers can appear between or
+     inside basic blocks.  If AFTER is the tail of its basic block, do
+     not set BLOCK_FOR_INSN on the new note.  */
+  else if ((subtype == NOTE_INSN_VAR_LOCATION
+	    || subtype == NOTE_INSN_CALL_ARG_LOCATION
+	    || subtype == NOTE_INSN_EH_REGION_BEG
+	    || subtype == NOTE_INSN_EH_REGION_END)
+	   && bb != NULL && BB_END (bb) == after)
+    add_insn_after_nobb (note, after);
+  else
+    add_insn_after (note, after, NULL);
+  return note;
+}
+
+/* Emit a note of subtype SUBTYPE before the insn BEFORE.  */
+
+rtx
+emit_note_before (enum insn_note subtype, rtx before)
+{
+  /* Some notes are never emitted.  */
+  gcc_assert (subtype != NOTE_INSN_DELETED_LABEL);
+
+  basic_block bb = BARRIER_P (before) ? NULL : BLOCK_FOR_INSN (before);
+  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)));
+
+  /* NOTE_INSN_SWITCH_TEXT_SECTIONS note only appears between basic blocks.  */
+  if (subtype == NOTE_INSN_SWITCH_TEXT_SECTIONS)
+    add_insn_before_nobb (note, before);
+  /* Notes for var tracking and EH region markers can appear between or
+     inside basic blocks.  If BEFORE is the head of its basic block, do
+     not set BLOCK_FOR_INSN on the new note.  */
+  else if ((subtype == NOTE_INSN_VAR_LOCATION
+	    || subtype == NOTE_INSN_CALL_ARG_LOCATION
+	    || subtype == NOTE_INSN_EH_REGION_BEG
+	    || subtype == NOTE_INSN_EH_REGION_END)
+	   && bb != NULL && BB_HEAD (bb) == before)
+    add_insn_before_nobb (note, before);
+  else
+    add_insn_before (note, before, NULL);
   return note;
 }
 
Index: bb-reorder.c
===================================================================
--- bb-reorder.c	(revision 197536)
+++ bb-reorder.c	(working copy)
@@ -2173,7 +2173,6 @@  static void
 insert_section_boundary_note (void)
 {
   basic_block bb;
-  rtx new_note;
   int first_partition = 0;
 
   if (!flag_reorder_blocks_and_partition)
@@ -2185,11 +2184,7 @@  insert_section_boundary_note (void)
 	first_partition = BB_PARTITION (bb);
       if (BB_PARTITION (bb) != first_partition)
 	{
-	  new_note = emit_note_before (NOTE_INSN_SWITCH_TEXT_SECTIONS,
-				       BB_HEAD (bb));
-	  /* ??? This kind of note always lives between basic blocks,
-	     but add_insn_before will set BLOCK_FOR_INSN anyway.  */
-	  BLOCK_FOR_INSN (new_note) = NULL;
+	  emit_note_before (NOTE_INSN_SWITCH_TEXT_SECTIONS, BB_HEAD (bb));
 	  break;
 	}
     }
Index: jump.c
===================================================================
--- jump.c	(revision 197536)
+++ jump.c	(working copy)
@@ -133,7 +133,7 @@  cleanup_barriers (void)
 	  if (BARRIER_P (prev))
 	    delete_insn (insn);
 	  else if (prev != PREV_INSN (insn))
-	    reorder_insns (insn, insn, prev);
+	    reorder_insns_nobb (insn, insn, prev);
 	}
     }
   return 0;
Index: var-tracking.c
===================================================================
--- var-tracking.c	(revision 197536)
+++ var-tracking.c	(working copy)
@@ -10238,6 +10238,7 @@  struct rtl_opt_pass pass_variable_tracki
   0,                                    /* properties_provided */
   0,                                    /* properties_destroyed */
   0,                                    /* todo_flags_start */
-  TODO_verify_rtl_sharing               /* todo_flags_finish */
+  TODO_verify_rtl_sharing
+   | TODO_verify_flow                   /* todo_flags_finish */
  }
 };