Patchwork Fix up var-tracking notes emitted in between bbs (PR middle-end/43631)

login
register
mail settings
Submitter Jakub Jelinek
Date Nov. 14, 2012, 11:02 p.m.
Message ID <20121114230229.GN1886@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/199091/
State New
Headers show

Comments

Jakub Jelinek - Nov. 14, 2012, 11:02 p.m.
Hi!

Steven has been complaining for some years that var-tracking inserts
NOTE_INSN_VAR_LOCATION (and NOTE_INSN_CALL_ARG_LOCATION) notes sometimes
in between basic blocks, but with BLOCK_FOR_INSN set (or sometimes extends
a bb ending originally with a noreturn or throwing call by a note or more
after it).  Fixed thusly, bootstrapped/regtested on x86_64-linux and
i686-linux, ok for trunk?

2012-11-14  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/43631
	* var-tracking.c (emit_note_insn_var_location, emit_notes_in_bb):
	Clear BLOCK_FOR_INSN on notes emitted in between basic blocks,
	don't adjust BB_END when inserting note after BB_END of some bb.


	Jakub
Steven Bosscher - Nov. 15, 2012, 9:52 p.m.
On Thu, Nov 15, 2012 at 12:02 AM, Jakub Jelinek wrote:
> Hi!
>
> Steven has been complaining for some years that var-tracking inserts
> NOTE_INSN_VAR_LOCATION (and NOTE_INSN_CALL_ARG_LOCATION) notes sometimes
> in between basic blocks, but with BLOCK_FOR_INSN set (or sometimes extends
> a bb ending originally with a noreturn or throwing call by a note or more
> after it).  Fixed thusly, bootstrapped/regtested on x86_64-linux and
> i686-linux, ok for trunk?

Many thanks again for fixing this!
Do you think it's safe to add a verify_flow_info call at the end of
var-tracking to make sure there will be no regressions from here?

In case people are curious why this is IMHO so important: This was the
last major obstacle for some ports to keep the CFG around until
'final'.

Ciao!
Steven
Alexandre Oliva - Nov. 18, 2012, 5:48 a.m.
On Nov 14, 2012, Jakub Jelinek <jakub@redhat.com> wrote:

> Steven has been complaining for some years that var-tracking inserts
> NOTE_INSN_VAR_LOCATION (and NOTE_INSN_CALL_ARG_LOCATION) notes sometimes
> in between basic blocks, but with BLOCK_FOR_INSN set (or sometimes extends
> a bb ending originally with a noreturn or throwing call by a note or more
> after it).  Fixed thusly, bootstrapped/regtested on x86_64-linux and
> i686-linux, ok for trunk?

This is ok with me, although I'm thinking it might make more sense to
consolidate the rules to decide whether or not to add basic block info
to a note in add_insn_{before,after}, where this added in the first
place.  Ideally we'd use the same rules used by cfg to determine where
the block boundaries are, i.e., what kinds of notes can be at BB
boundaries and which are outside BBs.  (Incidentally, it looks like
add_insn_after, unlike add_insn_before, completely ignores and
overwrites the bb it is passed as an argument; this seems wrong)
Steven Bosscher - Dec. 2, 2012, 12:55 p.m.
On Thu, Nov 15, 2012 at 12:02 AM, Jakub Jelinek wrote:
> 2012-11-14  Jakub Jelinek  < >
>
>         PR middle-end/43631
>         * var-tracking.c (emit_note_insn_var_location, emit_notes_in_bb):
>         Clear BLOCK_FOR_INSN on notes emitted in between basic blocks,
>         don't adjust BB_END when inserting note after BB_END of some bb.

Hello Jakub,

Please don't forget to commit this one! :-)

Ciao!
Steven

Patch

--- gcc/var-tracking.c.jj	2012-11-13 10:54:06.000000000 +0100
+++ gcc/var-tracking.c	2012-11-13 11:35:31.668284140 +0100
@@ -8566,9 +8566,30 @@  emit_note_insn_var_location (void **varp
 	      || NOTE_KIND (insn) == NOTE_INSN_CALL_ARG_LOCATION))
 	note = emit_note_after (NOTE_INSN_VAR_LOCATION, insn);
       else
-	note = emit_note_before (NOTE_INSN_VAR_LOCATION, insn);
+	{
+	  note = emit_note_before (NOTE_INSN_VAR_LOCATION, insn);
+	  /* If insn is BB_HEAD of some bb, make sure the note
+	     doesn't have BLOCK_FOR_INSN set.  The notes don't
+	     extend the extents of a basic block, and e.g. notes emitted
+	     for differences in between basic blocks should live in between
+	     the basic blocks.  */
+	  if (BLOCK_FOR_INSN (note)
+	      && BB_HEAD (BLOCK_FOR_INSN (note)) == insn)
+	    set_block_for_insn (note, NULL);
+	}
     }
   NOTE_VAR_LOCATION (note) = note_vl;
+  /* If insn is BB_END of some bb, make sure the note
+     doesn't have BLOCK_FOR_INSN set.  The notes don't
+     extend the extents of a basic block, and e.g. a noreturn
+     call can still be followed by NOTE_INSN_CALL_ARG_LOCATION.  */
+  if (BLOCK_FOR_INSN (note)
+      && BB_END (BLOCK_FOR_INSN (note)) == note
+      && PREV_INSN (note) == insn)
+    {
+      BB_END (BLOCK_FOR_INSN (note)) = insn;
+      set_block_for_insn (note, NULL);
+    }
 
   set_dv_changed (var->dv, false);
   gcc_assert (var->in_changed_variables);
@@ -8936,6 +8957,16 @@  emit_notes_in_bb (basic_block bb, datafl
 		}
 	      note = emit_note_after (NOTE_INSN_CALL_ARG_LOCATION, insn);
 	      NOTE_VAR_LOCATION (note) = arguments;
+	      /* If insn is BB_END of some bb, make sure the note
+		 doesn't have BLOCK_FOR_INSN set.  The notes don't
+		 extend the extents of a basic block, and e.g. a noreturn
+		 call can still be followed by NOTE_INSN_CALL_ARG_LOCATION.  */
+	      if (BLOCK_FOR_INSN (note)
+		  && BB_END (BLOCK_FOR_INSN (note)) == note)
+		{
+		  BB_END (BLOCK_FOR_INSN (note)) = insn;
+		  set_block_for_insn (note, NULL);
+		}
 	    }
 	    break;