diff mbox

Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3)

Message ID 20170324193616.GV11094@tucnak
State New
Headers show

Commit Message

Jakub Jelinek March 24, 2017, 7:36 p.m. UTC 
On Fri, Mar 24, 2017 at 11:56:05AM -0600, Jeff Law wrote:
> > We could e.g.
> > #ifndef REG_CFA_NOTE
> > # define REG_CFA_NOTE(NAME) REG_NOTE(NAME)
> > #endif
> > and then
> > REG_CFA_NOTE (FRAME_RELATED_EXPR)
> > etc. in reg-notes.def (and document that REG_CFA_NOTE should be used for
> > notes related to CFA).
> > Then in cfgcleanups.c we could just
> > #undef REG_CFA_NOTE
> > #define DEF_REG_NOTE(NAME)
> > #define REG_CFA_NOTE(NAME) REG_##NAME,
> > #include "reg-notes.def"
> > #undef DEF_REG_NOTE
> > #undef REG_CFA_NOTE
> > to populate the cfa_note_kinds array.
> Something like that seems preferable -- I think we're a lot more likely to
> catch the need to use REG_CFA_NOTE when defining the notes in reg-notes.def
> than we are to remember to update an array in a different file.

So like this (if it passes bootstrap/regtest on x86_64, i686 and
powerpc64le)?

2017-03-24  Jakub Jelinek  <jakub@redhat.com>

	PR target/80102
	* reg-notes.def (REG_CFA_NOTE): Define.  Use it for CFA related
	notes.
	* cfgcleanup.c (old_insns_match_p): Don't cross-jump in between /f
	and non-/f instructions.  If both i1 and i2 are frame related,
	verify all CFA notes, their order and content.

	* g++.dg/opt/pr80102.C: New test.



	Jakub

Comments

Jakub Jelinek March 24, 2017, 9:44 p.m. UTC | #1 
On Fri, Mar 24, 2017 at 08:36:16PM +0100, Jakub Jelinek wrote:
> On Fri, Mar 24, 2017 at 11:56:05AM -0600, Jeff Law wrote:
> > > We could e.g.
> > > #ifndef REG_CFA_NOTE
> > > # define REG_CFA_NOTE(NAME) REG_NOTE(NAME)
> > > #endif
> > > and then
> > > REG_CFA_NOTE (FRAME_RELATED_EXPR)
> > > etc. in reg-notes.def (and document that REG_CFA_NOTE should be used for
> > > notes related to CFA).
> > > Then in cfgcleanups.c we could just
> > > #undef REG_CFA_NOTE
> > > #define DEF_REG_NOTE(NAME)
> > > #define REG_CFA_NOTE(NAME) REG_##NAME,
> > > #include "reg-notes.def"
> > > #undef DEF_REG_NOTE
> > > #undef REG_CFA_NOTE
> > > to populate the cfa_note_kinds array.
> > Something like that seems preferable -- I think we're a lot more likely to
> > catch the need to use REG_CFA_NOTE when defining the notes in reg-notes.def
> > than we are to remember to update an array in a different file.
> 
> So like this (if it passes bootstrap/regtest on x86_64, i686 and
> powerpc64le)?
> 
> 2017-03-24  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/80102
> 	* reg-notes.def (REG_CFA_NOTE): Define.  Use it for CFA related
> 	notes.
> 	* cfgcleanup.c (old_insns_match_p): Don't cross-jump in between /f
> 	and non-/f instructions.  If both i1 and i2 are frame related,
> 	verify all CFA notes, their order and content.
> 
> 	* g++.dg/opt/pr80102.C: New test.

Successfully bootstrapped/regtested on {x86_64,i686,powerpc64le}-linux, ok
for trunk?

	Jakub
Segher Boessenkool March 24, 2017, 11:37 p.m. UTC | #2 
Hi!

On Fri, Mar 24, 2017 at 08:36:16PM +0100, Jakub Jelinek wrote:
> +	  /* Skip over reg notes not related to CFI information.  */
> +	  while (n1)
> +	    {
> +	      for (i = 0; i < ARRAY_SIZE (cfa_note_kinds) - 1; i++)
> +		if (REG_NOTE_KIND (n1) == cfa_note_kinds[i])
> +		  break;
> +	      if (i != ARRAY_SIZE (cfa_note_kinds))
> +		break;
> +	      n1 = XEXP (n1, 1);
> +	    }

Maybe factor out reg_note_is_cfa_note and/or insns_have_identical_cfa_notes
functions?

The patch looks fine to me btw.  Thanks for working on this!


Segher
Jakub Jelinek March 25, 2017, 6:45 a.m. UTC | #3 
On Fri, Mar 24, 2017 at 06:37:46PM -0500, Segher Boessenkool wrote:
> On Fri, Mar 24, 2017 at 08:36:16PM +0100, Jakub Jelinek wrote:
> > +	  /* Skip over reg notes not related to CFI information.  */
> > +	  while (n1)
> > +	    {
> > +	      for (i = 0; i < ARRAY_SIZE (cfa_note_kinds) - 1; i++)
> > +		if (REG_NOTE_KIND (n1) == cfa_note_kinds[i])
> > +		  break;
> > +	      if (i != ARRAY_SIZE (cfa_note_kinds))
> > +		break;
> > +	      n1 = XEXP (n1, 1);
> > +	    }
> 
> Maybe factor out reg_note_is_cfa_note and/or insns_have_identical_cfa_notes
> functions?

Well, for the reg_note_is_cfa_note, actually it might be better to just
have a const bool array indexed by the note enum instead of the loop, I'll
implement it later.  And yes, I can outline insns_have_identical_cfa_notes.

	Jakub
diff mbox

Patch

--- gcc/reg-notes.def.jj	2017-01-21 02:26:33.000000000 +0100
+++ gcc/reg-notes.def	2017-03-24 19:14:23.413483807 +0100
@@ -20,10 +20,16 @@  along with GCC; see the file COPYING3.
 /* This file defines all the codes that may appear on individual
    EXPR_LIST, INSN_LIST and INT_LIST rtxes in the REG_NOTES chain of an insn.
    The codes are stored in the mode field of the rtx.  Source files
-   define DEF_REG_NOTE appropriately before including this file.  */
+   define DEF_REG_NOTE appropriately before including this file.
+
+   CFA related notes meant for RTX_FRAME_RELATED_P instructions
+   should be declared with REG_CFA_NOTE macro instead of REG_NOTE.  */
 
 /* Shorthand.  */
 #define REG_NOTE(NAME) DEF_REG_NOTE (REG_##NAME)
+#ifndef REG_CFA_NOTE
+# define REG_CFA_NOTE(NAME) REG_NOTE (NAME)
+#endif
 
 /* REG_DEP_TRUE is used in scheduler dependencies lists to represent a
    read-after-write dependency (i.e. a true data dependency).  This is
@@ -112,7 +118,7 @@  REG_NOTE (BR_PRED)
 /* Attached to insns that are RTX_FRAME_RELATED_P, but are too complex
    for DWARF to interpret what they imply.  The attached rtx is used
    instead of intuition.  */
-REG_NOTE (FRAME_RELATED_EXPR)
+REG_CFA_NOTE (FRAME_RELATED_EXPR)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, but are too complex
    for FRAME_RELATED_EXPR intuition.  The insn's first pattern must be
@@ -122,7 +128,7 @@  REG_NOTE (FRAME_RELATED_EXPR)
    with a base register and a constant offset.  In the most complicated
    cases, this will result in a DW_CFA_def_cfa_expression with the rtx
    expression rendered in a dwarf location expression.  */
-REG_NOTE (CFA_DEF_CFA)
+REG_CFA_NOTE (CFA_DEF_CFA)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, but are too complex
    for FRAME_RELATED_EXPR intuition.  This note adjusts the expression
@@ -130,57 +136,57 @@  REG_NOTE (CFA_DEF_CFA)
    expression, relative to the old CFA expression.  This rtx must be of
    the form (SET new-cfa-reg (PLUS old-cfa-reg const_int)).  If the note
    rtx is NULL, we use the first SET of the insn.  */
-REG_NOTE (CFA_ADJUST_CFA)
+REG_CFA_NOTE (CFA_ADJUST_CFA)
 
 /* Similar to FRAME_RELATED_EXPR, with the additional information that
    this is a save to memory, i.e. will result in DW_CFA_offset or the
    like.  The pattern or the insn should be a simple store relative to
    the CFA.  */
-REG_NOTE (CFA_OFFSET)
+REG_CFA_NOTE (CFA_OFFSET)
 
 /* Similar to FRAME_RELATED_EXPR, with the additional information that this
    is a save to a register, i.e. will result in DW_CFA_register.  The insn
    or the pattern should be simple reg-reg move.  */
-REG_NOTE (CFA_REGISTER)
+REG_CFA_NOTE (CFA_REGISTER)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, but are too complex
    for FRAME_RELATED_EXPR intuition.  This is a save to memory, i.e. will
    result in a DW_CFA_expression.  The pattern or the insn should be a
    store of a register to an arbitrary (non-validated) memory address.  */
-REG_NOTE (CFA_EXPRESSION)
+REG_CFA_NOTE (CFA_EXPRESSION)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, but are too complex
    for FRAME_RELATED_EXPR intuition.  The DWARF expression computes the value of
    the given register.  */
-REG_NOTE (CFA_VAL_EXPRESSION)
+REG_CFA_NOTE (CFA_VAL_EXPRESSION)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, with the information
    that this is a restore operation, i.e. will result in DW_CFA_restore
    or the like.  Either the attached rtx, or the destination of the insn's
    first pattern is the register to be restored.  */
-REG_NOTE (CFA_RESTORE)
+REG_CFA_NOTE (CFA_RESTORE)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, marks insn that sets
    vDRAP from DRAP.  If vDRAP is a register, vdrap_reg is initalized
    to the argument, if it is a MEM, it is ignored.  */
-REG_NOTE (CFA_SET_VDRAP)
+REG_CFA_NOTE (CFA_SET_VDRAP)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, indicating a window
    save operation, i.e. will result in a DW_CFA_GNU_window_save.
    The argument is ignored.  */
-REG_NOTE (CFA_WINDOW_SAVE)
+REG_CFA_NOTE (CFA_WINDOW_SAVE)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, marks the insn as
    requiring that all queued information should be flushed *before* insn,
    regardless of what is visible in the rtl.  The argument is ignored.
    This is normally used for a call instruction which is not exposed to
    the rest of the compiler as a CALL_INSN.  */
-REG_NOTE (CFA_FLUSH_QUEUE)
+REG_CFA_NOTE (CFA_FLUSH_QUEUE)
 
 /* Attached to insns that are RTX_FRAME_RELATED_P, toggling the mangling status
    of return address.  Currently it's only used by AArch64.  The argument is
    ignored.  */
-REG_NOTE (CFA_TOGGLE_RA_MANGLE)
+REG_CFA_NOTE (CFA_TOGGLE_RA_MANGLE)
 
 /* Indicates what exception region an INSN belongs in.  This is used
    to indicate what region to which a call may throw.  REGION 0
--- gcc/cfgcleanup.c.jj	2017-03-22 19:31:37.682105201 +0100
+++ gcc/cfgcleanup.c	2017-03-24 19:31:07.861484754 +0100
@@ -1149,6 +1149,11 @@  old_insns_match_p (int mode ATTRIBUTE_UN
   else if (p1 || p2)
     return dir_none;
 
+  /* Do not allow cross-jumping between frame related insns and other
+     insns.  */
+  if (RTX_FRAME_RELATED_P (i1) != RTX_FRAME_RELATED_P (i2))
+    return dir_none;
+
   p1 = PATTERN (i1);
   p2 = PATTERN (i2);
 
@@ -1207,6 +1212,61 @@  old_insns_match_p (int mode ATTRIBUTE_UN
 	}
     }
 
+  /* If both i1 and i2 are frame related, verify all the CFA notes
+     in the same order and with the same content.  */
+  if (RTX_FRAME_RELATED_P (i1))
+    {
+      static enum reg_note cfa_note_kinds[] = {
+#undef REG_CFA_NOTE
+#define DEF_REG_NOTE(NAME)
+#define REG_CFA_NOTE(NAME) REG_##NAME,
+#include "reg-notes.def"
+#undef REG_CFA_NOTE
+#undef DEF_REG_NOTE
+	REG_NOTE_MAX
+      };
+      rtx n1 = REG_NOTES (i1);
+      rtx n2 = REG_NOTES (i2);
+      unsigned int i;
+      do
+	{
+	  /* Skip over reg notes not related to CFI information.  */
+	  while (n1)
+	    {
+	      for (i = 0; i < ARRAY_SIZE (cfa_note_kinds) - 1; i++)
+		if (REG_NOTE_KIND (n1) == cfa_note_kinds[i])
+		  break;
+	      if (i != ARRAY_SIZE (cfa_note_kinds))
+		break;
+	      n1 = XEXP (n1, 1);
+	    }
+	  while (n2)
+	    {
+	      for (i = 0; i < ARRAY_SIZE (cfa_note_kinds) - 1; i++)
+		if (REG_NOTE_KIND (n2) == cfa_note_kinds[i])
+		  break;
+	      if (i != ARRAY_SIZE (cfa_note_kinds))
+		break;
+	      n2 = XEXP (n2, 1);
+	    }
+	  if (n1 == NULL_RTX && n2 == NULL_RTX)
+	    break;
+	  if (n1 == NULL_RTX || n2 == NULL_RTX)
+	    return dir_none;
+	  if (XEXP (n1, 0) == XEXP (n2, 0))
+	    ;
+	  else if (XEXP (n1, 0) == NULL_RTX || XEXP (n2, 0) == NULL_RTX)
+	    return dir_none;
+	  else if (!(reload_completed
+		     ? rtx_renumbered_equal_p (XEXP (n1, 0), XEXP (n2, 0))
+		     : rtx_equal_p (XEXP (n1, 0), XEXP (n2, 0))))
+	    return dir_none;
+	  n1 = XEXP (n1, 1);
+	  n2 = XEXP (n2, 1);
+	}
+      while (1);
+    }
+
 #ifdef STACK_REGS
   /* If cross_jump_death_matters is not 0, the insn's mode
      indicates whether or not the insn contains any stack-like
--- gcc/testsuite/g++.dg/opt/pr80102.C.jj	2017-03-24 19:11:36.625645896 +0100
+++ gcc/testsuite/g++.dg/opt/pr80102.C	2017-03-24 19:11:36.625645896 +0100
@@ -0,0 +1,14 @@ 
+// PR target/80102
+// { dg-do compile }
+// { dg-options "-fnon-call-exceptions -Os" }
+// { dg-additional-options "-mminimal-toc" { target { powerpc*-*-* && lp64 } } }
+
+struct B { float a; B (float c) { for (int g; g < c;) ++a; } };
+struct D { D (B); };
+
+int
+main ()
+{
+  B (1.0);
+  D e (0.0), f (1.0);
+}