* [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102) @ 2017-03-20 21:15 Jakub Jelinek 2017-03-20 22:38 ` Richard Henderson 0 siblings, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2017-03-20 21:15 UTC (permalink / raw) To: gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102) 2017-03-20 21:15 [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102) Jakub Jelinek @ 2017-03-20 22:38 ` Richard Henderson 2017-03-21 7:41 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 2) Jakub Jelinek 0 siblings, 1 reply; 17+ messages in thread From: Richard Henderson @ 2017-03-20 22:38 UTC (permalink / raw) To: Jakub Jelinek, gcc-patches 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~ ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 2) 2017-03-20 22:38 ` Richard Henderson @ 2017-03-21 7:41 ` Jakub Jelinek 2017-03-21 17:53 ` Jakub Jelinek 0 siblings, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2017-03-21 7:41 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 2) 2017-03-21 7:41 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 2) Jakub Jelinek @ 2017-03-21 17:53 ` Jakub Jelinek 2017-03-21 20:21 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) Jakub Jelinek 0 siblings, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2017-03-21 17:53 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-21 17:53 ` Jakub Jelinek @ 2017-03-21 20:21 ` Jakub Jelinek 2017-03-24 12:39 ` Jeff Law 0 siblings, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2017-03-21 20:21 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-21 20:21 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) Jakub Jelinek @ 2017-03-24 12:39 ` Jeff Law 2017-03-24 13:08 ` Jakub Jelinek 0 siblings, 1 reply; 17+ messages in thread From: Jeff Law @ 2017-03-24 12:39 UTC (permalink / raw) To: Jakub Jelinek, Richard Henderson; +Cc: gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-24 12:39 ` Jeff Law @ 2017-03-24 13:08 ` Jakub Jelinek 2017-03-24 14:04 ` Jakub Jelinek 2017-03-24 18:10 ` Jeff Law 0 siblings, 2 replies; 17+ messages in thread From: Jakub Jelinek @ 2017-03-24 13:08 UTC (permalink / raw) To: Jeff Law; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-24 13:08 ` Jakub Jelinek @ 2017-03-24 14:04 ` Jakub Jelinek 2017-03-24 17:49 ` Jeff Law 2017-03-24 18:10 ` Jeff Law 1 sibling, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2017-03-24 14:04 UTC (permalink / raw) To: Jeff Law; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-24 14:04 ` Jakub Jelinek @ 2017-03-24 17:49 ` Jeff Law 2017-03-24 19:35 ` Jakub Jelinek 0 siblings, 1 reply; 17+ messages in thread From: Jeff Law @ 2017-03-24 17:49 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-24 17:49 ` Jeff Law @ 2017-03-24 19:35 ` Jakub Jelinek 0 siblings, 0 replies; 17+ messages in thread From: Jakub Jelinek @ 2017-03-24 19:35 UTC (permalink / raw) To: Jeff Law; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-24 13:08 ` Jakub Jelinek 2017-03-24 14:04 ` Jakub Jelinek @ 2017-03-24 18:10 ` Jeff Law 2017-03-24 19:42 ` Jakub Jelinek 1 sibling, 1 reply; 17+ messages in thread From: Jeff Law @ 2017-03-24 18:10 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-24 18:10 ` Jeff Law @ 2017-03-24 19:42 ` Jakub Jelinek 2017-03-24 22:04 ` Jakub Jelinek 2017-03-25 0:16 ` Segher Boessenkool 0 siblings, 2 replies; 17+ messages in thread From: Jakub Jelinek @ 2017-03-24 19:42 UTC (permalink / raw) To: Jeff Law; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-24 19:42 ` Jakub Jelinek @ 2017-03-24 22:04 ` Jakub Jelinek 2017-03-25 0:16 ` Segher Boessenkool 1 sibling, 0 replies; 17+ messages in thread From: Jakub Jelinek @ 2017-03-24 22:04 UTC (permalink / raw) To: Jeff Law; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-24 19:42 ` Jakub Jelinek 2017-03-24 22:04 ` Jakub Jelinek @ 2017-03-25 0:16 ` Segher Boessenkool 2017-03-25 8:54 ` Jakub Jelinek 1 sibling, 1 reply; 17+ messages in thread From: Segher Boessenkool @ 2017-03-25 0:16 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jeff Law, Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) 2017-03-25 0:16 ` Segher Boessenkool @ 2017-03-25 8:54 ` Jakub Jelinek 2017-03-26 22:06 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 4) Jakub Jelinek 0 siblings, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2017-03-25 8:54 UTC (permalink / raw) To: Segher Boessenkool; +Cc: Jeff Law, Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 4) 2017-03-25 8:54 ` Jakub Jelinek @ 2017-03-26 22:06 ` Jakub Jelinek 2017-03-27 18:15 ` Jeff Law 0 siblings, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2017-03-26 22:06 UTC (permalink / raw) To: Segher Boessenkool, Jeff Law; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 4) 2017-03-26 22:06 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 4) Jakub Jelinek @ 2017-03-27 18:15 ` Jeff Law 0 siblings, 0 replies; 17+ messages in thread From: Jeff Law @ 2017-03-27 18:15 UTC (permalink / raw) To: Jakub Jelinek, Segher Boessenkool; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-03-27 18:02 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-20 21:15 [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102) Jakub Jelinek 2017-03-20 22:38 ` Richard Henderson 2017-03-21 7:41 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 2) Jakub Jelinek 2017-03-21 17:53 ` Jakub Jelinek 2017-03-21 20:21 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 3) Jakub Jelinek 2017-03-24 12:39 ` Jeff Law 2017-03-24 13:08 ` Jakub Jelinek 2017-03-24 14:04 ` Jakub Jelinek 2017-03-24 17:49 ` Jeff Law 2017-03-24 19:35 ` Jakub Jelinek 2017-03-24 18:10 ` Jeff Law 2017-03-24 19:42 ` Jakub Jelinek 2017-03-24 22:04 ` Jakub Jelinek 2017-03-25 0:16 ` Segher Boessenkool 2017-03-25 8:54 ` Jakub Jelinek 2017-03-26 22:06 ` [PATCH] Don't cross-jump in between frame related and non-frame related insns (PR target/80102, take 4) Jakub Jelinek 2017-03-27 18:15 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).