[PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102)

[PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102)

[Jakub Jelinek]

Hi!

On ppc64le we ICE on the following testcase, because during jump2 we decide
to cross-jump
(insn/f 62 61 63 5 (unspec_volatile [
            (const_int 0 [0])
        ] UNSPECV_BLOCK) "pr80102.C":9 -1
     (expr_list:REG_CFA_RESTORE (reg:DI 30 30)
        (nil)))
(jump_insn 63 62 90 5 (simple_return) "pr80102.C":9 -1
     (nil)
with
(insn 73 72 84 8 (unspec_volatile [
            (const_int 0 [0])
        ] UNSPECV_BLOCK) "pr80102.C":9 504 {blockage}
     (nil))
(jump_insn 84 73 85 8 (simple_return) "pr80102.C":9 -1
     (nil)
Note that one of the blockage insns holds CFI instructions on them, while
the other doesn't (admittedly it is very weird to attach this to an empty
insn, but that is what the rs6000 backend does).

The following patch fixes that by avoiding to cross-jump them, because
that will often read to unwind info inconsistencies.
Not really sure what we should do if both i1 and i2 are frame related, shall
we check for each of the CFA reg notes if they are available and equal?
Or punt if either of the insns is frame related?

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

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

	PR target/80102
	* cfgcleanup.c (old_insns_match_p): Don't cross-jump in between /f
	and non-/f instructions.

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

--- gcc/cfgcleanup.c.jj	2017-01-09 22:46:03.000000000 +0100
+++ gcc/cfgcleanup.c	2017-03-20 13:55:58.823983848 +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);
 
--- gcc/testsuite/g++.dg/opt/pr80102.C.jj	2017-03-20 14:34:01.223434828 +0100
+++ gcc/testsuite/g++.dg/opt/pr80102.C	2017-03-20 14:33:36.000000000 +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);
+}

	Jakub

Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102)

[Richard Henderson]

On 03/21/2017 07:15 AM, Jakub Jelinek wrote:
> Not really sure what we should do if both i1 and i2 are frame related, shall
> we check for each of the CFA reg notes if they are available and equal?
> Or punt if either of the insns is frame related?

I would punt if either is frame related.

As an aside, if the length of "blockage" is corrected to 0, does cross-jumping 
skip this case?  Because replacing a simple_return with a direct branch to a 
simple_return is not a win.  But of course at the moment cross-jumping thinks 
it is eliminating the second blockage as well...


r~

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

[Jakub Jelinek]

On Tue, Mar 21, 2017 at 08:38:20AM +1000, Richard Henderson wrote:
> On 03/21/2017 07:15 AM, Jakub Jelinek wrote:
> > Not really sure what we should do if both i1 and i2 are frame related, shall
> > we check for each of the CFA reg notes if they are available and equal?
> > Or punt if either of the insns is frame related?
> 
> I would punt if either is frame related.

Ok, I'll test then the following patch and gather some statistic on how
often we trigger this.

> As an aside, if the length of "blockage" is corrected to 0, does
> cross-jumping skip this case?  Because replacing a simple_return with a
> direct branch to a simple_return is not a win.  But of course at the moment
> cross-jumping thinks it is eliminating the second blockage as well...

I don't think cross-jumping counts anything but the number of active_insn_p,
at least I can't find any get_attr_length uses outside of final.c in the
generic code (and just very few (shrink-wrap and bb-reorder)
get_attr_min_length calls).  So shall cross-jumping only count
ifdef HAVE_attr_length insns with get_attr_length > 0?

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

	PR target/80102
	* cfgcleanup.c (old_insns_match_p): Don't cross-jump frame related
	insns.

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

--- gcc/cfgcleanup.c.jj	2017-01-09 22:46:03.000000000 +0100
+++ gcc/cfgcleanup.c	2017-03-20 13:55:58.823983848 +0100
@@ -1149,6 +1149,10 @@ old_insns_match_p (int mode ATTRIBUTE_UN
   else if (p1 || p2)
     return dir_none;
 
+  /* Do not allow cross-jumping frame related insns.  */
+  if (RTX_FRAME_RELATED_P (i1) || RTX_FRAME_RELATED_P (i2))
+    return dir_none;
+
   p1 = PATTERN (i1);
   p2 = PATTERN (i2);
 
--- gcc/testsuite/g++.dg/opt/pr80102.C.jj	2017-03-20 14:34:01.223434828 +0100
+++ gcc/testsuite/g++.dg/opt/pr80102.C	2017-03-20 14:33:36.000000000 +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);
+}


	Jakub

Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 2)

[Jakub Jelinek]

On Tue, Mar 21, 2017 at 08:41:43AM +0100, Jakub Jelinek wrote:
> On Tue, Mar 21, 2017 at 08:38:20AM +1000, Richard Henderson wrote:
> > On 03/21/2017 07:15 AM, Jakub Jelinek wrote:
> > > Not really sure what we should do if both i1 and i2 are frame related, shall
> > > we check for each of the CFA reg notes if they are available and equal?
> > > Or punt if either of the insns is frame related?
> > 
> > I would punt if either is frame related.
> 
> Ok, I'll test then the following patch and gather some statistic on how
> often we trigger this.

The statistics I've gathered unfortunately shows that at least on
powerpc64le-linux it is very important to not give up if both i1 and i2
are frame related and have rtx_equal_p notes.
I've set on unpatched old_insns_match_p flags when returning non-dir_none
and checked those flags in the various callers of these when about to
successfully perform cross-jumping, head-merging etc.
With /f vs. non-/f, the only 3 hits were on the new pr80102.C testcase
during powerpc64le-linux bootstrap/regtest, but /f vs. /f there were
167601 hits.

	Jakub

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

[Jakub Jelinek]

On Tue, Mar 21, 2017 at 06:53:34PM +0100, Jakub Jelinek wrote:
> On Tue, Mar 21, 2017 at 08:41:43AM +0100, Jakub Jelinek wrote:
> > On Tue, Mar 21, 2017 at 08:38:20AM +1000, Richard Henderson wrote:
> > > On 03/21/2017 07:15 AM, Jakub Jelinek wrote:
> > > > Not really sure what we should do if both i1 and i2 are frame related, shall
> > > > we check for each of the CFA reg notes if they are available and equal?
> > > > Or punt if either of the insns is frame related?
> > > 
> > > I would punt if either is frame related.
> > 
> > Ok, I'll test then the following patch and gather some statistic on how
> > often we trigger this.
> 
> The statistics I've gathered unfortunately shows that at least on
> powerpc64le-linux it is very important to not give up if both i1 and i2
> are frame related and have rtx_equal_p notes.
> I've set on unpatched old_insns_match_p flags when returning non-dir_none
> and checked those flags in the various callers of these when about to
> successfully perform cross-jumping, head-merging etc.
> With /f vs. non-/f, the only 3 hits were on the new pr80102.C testcase
> during powerpc64le-linux bootstrap/regtest, but /f vs. /f there were
> 167601 hits.

Here is updated patch which allows cross-jumping of RTX_FRAME_RELATED_P
insns, as long as they have the same CFA related notes on them (same order
if more than one).

Bootstrapped/regtested on powerpc64le-linux, ok for trunk?

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

	PR target/80102
	* 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.

--- gcc/cfgcleanup.c.jj	2017-03-21 07:56:55.711233924 +0100
+++ gcc/cfgcleanup.c	2017-03-21 19:20:40.517746664 +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,58 @@ 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[] = {
+	REG_FRAME_RELATED_EXPR, REG_CFA_DEF_CFA, REG_CFA_ADJUST_CFA,
+	REG_CFA_OFFSET, REG_CFA_REGISTER, REG_CFA_EXPRESSION,
+	REG_CFA_VAL_EXPRESSION, REG_CFA_RESTORE, REG_CFA_SET_VDRAP,
+	REG_CFA_TOGGLE_RA_MANGLE, REG_CFA_WINDOW_SAVE, REG_CFA_FLUSH_QUEUE
+      };
+      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); 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); 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-21 18:56:30.256698365 +0100
+++ gcc/testsuite/g++.dg/opt/pr80102.C	2017-03-21 18:56:30.256698365 +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);
+}


	Jakub

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

[Jeff Law]

On 03/21/2017 02:21 PM, Jakub Jelinek wrote:
> On Tue, Mar 21, 2017 at 06:53:34PM +0100, Jakub Jelinek wrote:
>> On Tue, Mar 21, 2017 at 08:41:43AM +0100, Jakub Jelinek wrote:
>>> On Tue, Mar 21, 2017 at 08:38:20AM +1000, Richard Henderson wrote:
>>>> On 03/21/2017 07:15 AM, Jakub Jelinek wrote:
>>>>> Not really sure what we should do if both i1 and i2 are frame related, shall
>>>>> we check for each of the CFA reg notes if they are available and equal?
>>>>> Or punt if either of the insns is frame related?
>>>>
>>>> I would punt if either is frame related.
>>>
>>> Ok, I'll test then the following patch and gather some statistic on how
>>> often we trigger this.
>>
>> The statistics I've gathered unfortunately shows that at least on
>> powerpc64le-linux it is very important to not give up if both i1 and i2
>> are frame related and have rtx_equal_p notes.
>> I've set on unpatched old_insns_match_p flags when returning non-dir_none
>> and checked those flags in the various callers of these when about to
>> successfully perform cross-jumping, head-merging etc.
>> With /f vs. non-/f, the only 3 hits were on the new pr80102.C testcase
>> during powerpc64le-linux bootstrap/regtest, but /f vs. /f there were
>> 167601 hits.
>
> Here is updated patch which allows cross-jumping of RTX_FRAME_RELATED_P
> insns, as long as they have the same CFA related notes on them (same order
> if more than one).
>
> Bootstrapped/regtested on powerpc64le-linux, ok for trunk?
>
> 2017-03-21  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/80102
> 	* 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.
Presumably this didn't ICE at some point in the past, so it's a 
regression?  (it's not marked as such in the BZ).


>
> --- gcc/cfgcleanup.c.jj	2017-03-21 07:56:55.711233924 +0100
> +++ gcc/cfgcleanup.c	2017-03-21 19:20:40.517746664 +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,58 @@ 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[] = {
> +	REG_FRAME_RELATED_EXPR, REG_CFA_DEF_CFA, REG_CFA_ADJUST_CFA,
> +	REG_CFA_OFFSET, REG_CFA_REGISTER, REG_CFA_EXPRESSION,
> +	REG_CFA_VAL_EXPRESSION, REG_CFA_RESTORE, REG_CFA_SET_VDRAP,
> +	REG_CFA_TOGGLE_RA_MANGLE, REG_CFA_WINDOW_SAVE, REG_CFA_FLUSH_QUEUE
> +      };
ISTM this could get out of date very easily.  Is there a clean way to 
generate the array of cfa notes as we build up the notes from 
reg-notes.def?


Jeff

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

[Jakub Jelinek]

On Fri, Mar 24, 2017 at 06:37:10AM -0600, Jeff Law wrote:
> > 2017-03-21  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR target/80102
> > 	* 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.
> Presumably this didn't ICE at some point in the past, so it's a regression?
> (it's not marked as such in the BZ).

It doesn't ICE for me with r238210 and ICEs with current trunk, I don't have
too many ppc64le compilers around though.

> > +  /* 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[] = {
> > +	REG_FRAME_RELATED_EXPR, REG_CFA_DEF_CFA, REG_CFA_ADJUST_CFA,
> > +	REG_CFA_OFFSET, REG_CFA_REGISTER, REG_CFA_EXPRESSION,
> > +	REG_CFA_VAL_EXPRESSION, REG_CFA_RESTORE, REG_CFA_SET_VDRAP,
> > +	REG_CFA_TOGGLE_RA_MANGLE, REG_CFA_WINDOW_SAVE, REG_CFA_FLUSH_QUEUE
> > +      };
> ISTM this could get out of date very easily.  Is there a clean way to
> generate the array of cfa notes as we build up the notes from reg-notes.def?

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.

	Jakub

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

[Jakub Jelinek]

On Fri, Mar 24, 2017 at 02:04:42PM +0100, Jakub Jelinek wrote:
> On Fri, Mar 24, 2017 at 06:37:10AM -0600, Jeff Law wrote:
> > > 2017-03-21  Jakub Jelinek  <jakub@redhat.com>
> > > 
> > > 	PR target/80102
> > > 	* 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.
> > Presumably this didn't ICE at some point in the past, so it's a regression?
> > (it's not marked as such in the BZ).
> 
> It doesn't ICE for me with r238210 and ICEs with current trunk, I don't have
> too many ppc64le compilers around though.

GCC 4.8 doesn't ICE either.

	Jakub

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

[Jeff Law]

On 03/24/2017 07:35 AM, Jakub Jelinek wrote:
> On Fri, Mar 24, 2017 at 02:04:42PM +0100, Jakub Jelinek wrote:
>> On Fri, Mar 24, 2017 at 06:37:10AM -0600, Jeff Law wrote:
>>>> 2017-03-21  Jakub Jelinek  <jakub@redhat.com>
>>>>
>>>> 	PR target/80102
>>>> 	* 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.
>>> Presumably this didn't ICE at some point in the past, so it's a regression?
>>> (it's not marked as such in the BZ).
>>
>> It doesn't ICE for me with r238210 and ICEs with current trunk, I don't have
>> too many ppc64le compilers around though.
>
> GCC 4.8 doesn't ICE either.
ISTM this isn't a regression (without doing deeper analysis).  As a 
release manager, ISTM you can grant an exception if you want to push 
this forward for gcc-7.

Jeff

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

[Jeff Law]

On 03/24/2017 07:04 AM, Jakub Jelinek wrote:
> On Fri, Mar 24, 2017 at 06:37:10AM -0600, Jeff Law wrote:
>>> 2017-03-21  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR target/80102
>>> 	* 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.
>> Presumably this didn't ICE at some point in the past, so it's a regression?
>> (it's not marked as such in the BZ).
>
> It doesn't ICE for me with r238210 and ICEs with current trunk, I don't have
> too many ppc64le compilers around though.
>
>>> +  /* 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[] = {
>>> +	REG_FRAME_RELATED_EXPR, REG_CFA_DEF_CFA, REG_CFA_ADJUST_CFA,
>>> +	REG_CFA_OFFSET, REG_CFA_REGISTER, REG_CFA_EXPRESSION,
>>> +	REG_CFA_VAL_EXPRESSION, REG_CFA_RESTORE, REG_CFA_SET_VDRAP,
>>> +	REG_CFA_TOGGLE_RA_MANGLE, REG_CFA_WINDOW_SAVE, REG_CFA_FLUSH_QUEUE
>>> +      };
>> ISTM this could get out of date very easily.  Is there a clean way to
>> generate the array of cfa notes as we build up the notes from reg-notes.def?
>
> 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.

jeff

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

[Jakub Jelinek]

On Fri, Mar 24, 2017 at 11:47:35AM -0600, Jeff Law wrote:
> On 03/24/2017 07:35 AM, Jakub Jelinek wrote:
> > On Fri, Mar 24, 2017 at 02:04:42PM +0100, Jakub Jelinek wrote:
> > > On Fri, Mar 24, 2017 at 06:37:10AM -0600, Jeff Law wrote:
> > > > > 2017-03-21  Jakub Jelinek  <jakub@redhat.com>
> > > > > 
> > > > > 	PR target/80102
> > > > > 	* 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.
> > > > Presumably this didn't ICE at some point in the past, so it's a regression?
> > > > (it's not marked as such in the BZ).
> > > 
> > > It doesn't ICE for me with r238210 and ICEs with current trunk, I don't have
> > > too many ppc64le compilers around though.
> > 
> > GCC 4.8 doesn't ICE either.
> ISTM this isn't a regression (without doing deeper analysis).  As a release
> manager, ISTM you can grant an exception if you want to push this forward
> for gcc-7.

It is a [7 Regression], started with r239866, so it is a P1.

	Jakub

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

[Jakub Jelinek]

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.

--- 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);
+}


	Jakub

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

[Jakub Jelinek]

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

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

[Segher Boessenkool]

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

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

[Jakub Jelinek]

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

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

[Jakub Jelinek]

Hi!

On Sat, Mar 25, 2017 at 07:45:53AM +0100, Jakub Jelinek wrote:
> 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.

Here is updated patch that does that, bootstrapped/regtested on
powerpc64le-linux, ok for trunk?

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

	PR target/80102
	* reg-notes.def (REG_CFA_NOTE): Define.  Use it for CFA related
	notes.
	* cfgcleanup.c (reg_note_cfa_p): New array.
	(insns_have_identical_cfa_notes): New function.
	(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.

--- gcc/reg-notes.def.jj	2017-03-24 22:42:29.306844194 +0100
+++ gcc/reg-notes.def	2017-03-26 19:57:22.220346563 +0200
@@ -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-24 22:42:29.624840140 +0100
+++ gcc/cfgcleanup.c	2017-03-26 20:16:19.825581466 +0200
@@ -1111,6 +1111,48 @@ merge_dir (enum replace_direction a, enu
   return dir_none;
 }
 
+/* Array of flags indexed by reg note kind, true if the given
+   reg note is CFA related.  */
+static const bool reg_note_cfa_p[] = {
+#undef REG_CFA_NOTE
+#define DEF_REG_NOTE(NAME) false,
+#define REG_CFA_NOTE(NAME) true,
+#include "reg-notes.def"
+#undef REG_CFA_NOTE
+#undef DEF_REG_NOTE
+  false
+};
+
+/* Return true if I1 and I2 have identical CFA notes (the same order
+   and equivalent content).  */
+
+static bool
+insns_have_identical_cfa_notes (rtx_insn *i1, rtx_insn *i2)
+{
+  rtx n1, n2;
+  for (n1 = REG_NOTES (i1), n2 = REG_NOTES (i2); ;
+       n1 = XEXP (n1, 1), n2 = XEXP (n2, 1))
+    {
+      /* Skip over reg notes not related to CFI information.  */
+      while (n1 && !reg_note_cfa_p[REG_NOTE_KIND (n1)])
+	n1 = XEXP (n1, 1);
+      while (n2 && !reg_note_cfa_p[REG_NOTE_KIND (n2)])
+	n2 = XEXP (n2, 1);
+      if (n1 == NULL_RTX && n2 == NULL_RTX)
+	return true;
+      if (n1 == NULL_RTX || n2 == NULL_RTX)
+	return false;
+      if (XEXP (n1, 0) == XEXP (n2, 0))
+	;
+      else if (XEXP (n1, 0) == NULL_RTX || XEXP (n2, 0) == NULL_RTX)
+	return false;
+      else if (!(reload_completed
+		 ? rtx_renumbered_equal_p (XEXP (n1, 0), XEXP (n2, 0))
+		 : rtx_equal_p (XEXP (n1, 0), XEXP (n2, 0))))
+	return false;
+    }
+}
+
 /* Examine I1 and I2 and return:
    - dir_forward if I1 can be replaced by I2, or
    - dir_backward if I2 can be replaced by I1, or
@@ -1149,6 +1191,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 +1254,11 @@ 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) && !insns_have_identical_cfa_notes (i1, i2))
+    return dir_none;
+
 #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-26 19:57:22.221346550 +0200
+++ gcc/testsuite/g++.dg/opt/pr80102.C	2017-03-26 19:57:22.221346550 +0200
@@ -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);
+}


	Jakub

Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 4)

[Jeff Law]

On 03/26/2017 03:00 PM, Jakub Jelinek wrote:
> Hi!
>
> On Sat, Mar 25, 2017 at 07:45:53AM +0100, Jakub Jelinek wrote:
>> 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.
>
> Here is updated patch that does that, bootstrapped/regtested on
> powerpc64le-linux, ok for trunk?
>
> 2017-03-26  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/80102
> 	* reg-notes.def (REG_CFA_NOTE): Define.  Use it for CFA related
> 	notes.
> 	* cfgcleanup.c (reg_note_cfa_p): New array.
> 	(insns_have_identical_cfa_notes): New function.
> 	(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.
OK.
jeff