* Patch ping
@ 2012-11-16 9:10 Jakub Jelinek
2012-11-17 19:12 ` Richard Henderson
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jakub Jelinek @ 2012-11-16 9:10 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Henderson, Jeff Law
Hi!
- PR54921 invalidate sp in cselib on fp setter insn
http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02035.html
(perhaps in light of PR54402 the single_succ (ENTRY_BLOCK_PTR)
from the patch should be nuked)
- PR55188 testcase fix for targets with different branch cost
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00361.html
- PR55137 fold reassociation fix
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00636.html
(alternatively also retry with unsigned atype if the reassociation
would fail otherwise)
- PR55236 range test optimization fix with -fwrapv
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00707.html
(perhaps PLUS_EXPR can be handled similarly instead of bailing out)
Jakub
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: Patch ping 2012-11-16 9:10 Patch ping Jakub Jelinek @ 2012-11-17 19:12 ` Richard Henderson 2012-11-17 19:16 ` Richard Henderson 2012-11-17 20:04 ` Richard Henderson 2 siblings, 0 replies; 10+ messages in thread From: Richard Henderson @ 2012-11-17 19:12 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, Jeff Law On 11/16/2012 01:10 AM, Jakub Jelinek wrote: > - PR55188 testcase fix for targets with different branch cost > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00361.html Ok. r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patch ping 2012-11-16 9:10 Patch ping Jakub Jelinek 2012-11-17 19:12 ` Richard Henderson @ 2012-11-17 19:16 ` Richard Henderson 2012-11-17 20:04 ` Richard Henderson 2 siblings, 0 replies; 10+ messages in thread From: Richard Henderson @ 2012-11-17 19:16 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, Jeff Law On 11/16/2012 01:10 AM, Jakub Jelinek wrote: > - PR55236 range test optimization fix with -fwrapv > http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00707.html > (perhaps PLUS_EXPR can be handled similarly instead of bailing out) > Ok. r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patch ping 2012-11-16 9:10 Patch ping Jakub Jelinek 2012-11-17 19:12 ` Richard Henderson 2012-11-17 19:16 ` Richard Henderson @ 2012-11-17 20:04 ` Richard Henderson 2012-11-19 7:53 ` Jakub Jelinek 2 siblings, 1 reply; 10+ messages in thread From: Richard Henderson @ 2012-11-17 20:04 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, Jeff Law On 11/16/2012 01:10 AM, Jakub Jelinek wrote: > Hi! > > - PR54921 invalidate sp in cselib on fp setter insn > http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02035.html > (perhaps in light of PR54402 the single_succ (ENTRY_BLOCK_PTR) > from the patch should be nuked) > + rtx expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX); > + if (expr) > + pat = XEXP (expr, 0); > + if (GET_CODE (pat) == SET > + && SET_DEST (pat) == hard_frame_pointer_rtx) > + cselib_invalidate_rtx (stack_pointer_rtx); > + else if (GET_CODE (pat) == PARALLEL) This is only one possible way that FP can be set. The others are with CFA_DEF_CFA or CFA_ADJUST_CFA. Given + && frame_pointer_needed + && RTX_FRAME_RELATED_P (insn) is there any reason to do more than && modified_in_p (hard_frame_pointer_rtx, insn) ? r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patch ping 2012-11-17 20:04 ` Richard Henderson @ 2012-11-19 7:53 ` Jakub Jelinek 2012-11-19 16:56 ` Richard Henderson 0 siblings, 1 reply; 10+ messages in thread From: Jakub Jelinek @ 2012-11-19 7:53 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches, Jeff Law On Sat, Nov 17, 2012 at 11:10:04AM -0800, Richard Henderson wrote: > On 11/16/2012 01:10 AM, Jakub Jelinek wrote: > > Hi! > > > > - PR54921 invalidate sp in cselib on fp setter insn > > http://gcc.gnu.org/ml/gcc-patches/2012-10/msg02035.html > > (perhaps in light of PR54402 the single_succ (ENTRY_BLOCK_PTR) > > from the patch should be nuked) > > > + rtx expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX); > > + if (expr) > > + pat = XEXP (expr, 0); > > + if (GET_CODE (pat) == SET > > + && SET_DEST (pat) == hard_frame_pointer_rtx) > > + cselib_invalidate_rtx (stack_pointer_rtx); > > + else if (GET_CODE (pat) == PARALLEL) > > This is only one possible way that FP can be set. > The others are with CFA_DEF_CFA or CFA_ADJUST_CFA. > > Given > > + && frame_pointer_needed > + && RTX_FRAME_RELATED_P (insn) > > is there any reason to do more than > > && modified_in_p (hard_frame_pointer_rtx, insn) > > ? I'd prefer to only invalidate the stack pointer on the first assignment to the hard pointer. If the cselib link between sp and hfp is already broken, invalidating sp will only result in worse code. Dunno if there are any targets that adjust the hard frame pointer after it has been set once or similar. Perhaps we could walk here CSELIB_VAL_PTR (hfpval)->locs here, and look if any rtls in there have find_base_term (x->loc) == find_base_term (stack_pointer_rtx), and only if yes, invalidate (and guard it by the modified_in_p test). BTW, var-tracking.c uses a similar test. Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Patch ping 2012-11-19 7:53 ` Jakub Jelinek @ 2012-11-19 16:56 ` Richard Henderson 2012-11-19 20:40 ` [PATCH] Invalidate in cselib sp after processing frame_pointer_needed fp setter (PR rtl-optimization/54921, take 2) Jakub Jelinek 0 siblings, 1 reply; 10+ messages in thread From: Richard Henderson @ 2012-11-19 16:56 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, Jeff Law On 2012-11-18 23:53, Jakub Jelinek wrote: > I'd prefer to only invalidate the stack pointer on the first assignment > to the hard pointer. If the cselib link between sp and hfp is already > broken, invalidating sp will only result in worse code. Dunno if there > are any targets that adjust the hard frame pointer after it has been set > once or similar. I'm not aware of such a target. Though I did not audit them now. > Perhaps we could walk here CSELIB_VAL_PTR (hfpval)->locs here, and look > if any rtls in there have find_base_term (x->loc) == find_base_term > (stack_pointer_rtx), and only if yes, invalidate (and guard it by the > modified_in_p test). Sounds plausible. > BTW, var-tracking.c uses a similar test. Ouch. Where is that? r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Invalidate in cselib sp after processing frame_pointer_needed fp setter (PR rtl-optimization/54921, take 2) 2012-11-19 16:56 ` Richard Henderson @ 2012-11-19 20:40 ` Jakub Jelinek 2012-11-19 21:42 ` Richard Henderson 0 siblings, 1 reply; 10+ messages in thread From: Jakub Jelinek @ 2012-11-19 20:40 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches, Jeff Law On Mon, Nov 19, 2012 at 08:56:22AM -0800, Richard Henderson wrote: > On 2012-11-18 23:53, Jakub Jelinek wrote: > > I'd prefer to only invalidate the stack pointer on the first assignment > > to the hard pointer. If the cselib link between sp and hfp is already > > broken, invalidating sp will only result in worse code. Dunno if there > > are any targets that adjust the hard frame pointer after it has been set > > once or similar. > > I'm not aware of such a target. Though I did not audit them now. > > > Perhaps we could walk here CSELIB_VAL_PTR (hfpval)->locs here, and look > > if any rtls in there have find_base_term (x->loc) == find_base_term > > (stack_pointer_rtx), and only if yes, invalidate (and guard it by the > > modified_in_p test). > > Sounds plausible. > > > BTW, var-tracking.c uses a similar test. > > Ouch. Where is that? Here is an updated patch, bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2012-11-19 Jakub Jelinek <jakub@redhat.com> PR rtl-optimization/54921 * cselib.h (fp_setter_insn): New prototype. * cselib.c (fp_setter_insn): New function. (cselib_process_insn): If frame_pointer_needed, call cselib_invalidate_rtx (stack_pointer_rtx) after processing a frame pointer setter. * var-tracking.c (fp_setter): Removed. (vt_initialize): Use fp_setter_insn instead of fp_setter. * gcc.dg/pr54921.c: New test. --- gcc/cselib.h.jj 2012-10-16 13:20:25.000000000 +0200 +++ gcc/cselib.h 2012-11-19 19:02:27.768645544 +0100 @@ -78,6 +78,7 @@ extern void cselib_init (int); extern void cselib_clear_table (void); extern void cselib_finish (void); extern void cselib_process_insn (rtx); +extern bool fp_setter_insn (rtx); extern enum machine_mode cselib_reg_set_mode (const_rtx); extern int rtx_equal_for_cselib_p (rtx, rtx); extern int references_value_p (const_rtx, int); --- gcc/cselib.c.jj 2012-11-12 11:23:02.579098220 +0100 +++ gcc/cselib.c 2012-11-19 19:16:25.406747280 +0100 @@ -2593,6 +2593,28 @@ cselib_record_sets (rtx insn) } } +/* Return true if INSN in the prologue initializes hard_frame_pointer_rtx. */ + +bool +fp_setter_insn (rtx insn) +{ + rtx expr, pat = NULL_RTX; + + if (!RTX_FRAME_RELATED_P (insn)) + return false; + + expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX); + if (expr) + pat = XEXP (expr, 0); + if (!modified_in_p (hard_frame_pointer_rtx, pat ? pat : insn)) + return false; + + /* Don't return true for frame pointer restores in the epilogue. */ + if (find_reg_note (insn, REG_CFA_RESTORE, hard_frame_pointer_rtx)) + return false; + return true; +} + /* Record the effects of INSN. */ void @@ -2651,6 +2673,15 @@ cselib_process_insn (rtx insn) if (GET_CODE (XEXP (x, 0)) == CLOBBER) cselib_invalidate_rtx (XEXP (XEXP (x, 0), 0)); + /* On setter of the hard frame pointer if frame_pointer_needed, + invalidate stack_pointer_rtx, so that sp and {,h}fp based + VALUEs are distinct. */ + if (reload_completed + && frame_pointer_needed + && RTX_FRAME_RELATED_P (insn) + && fp_setter_insn (insn)) + cselib_invalidate_rtx (stack_pointer_rtx); + cselib_current_insn = NULL_RTX; if (n_useless_values > MAX_USELESS_VALUES --- gcc/var-tracking.c.jj 2012-11-19 14:41:26.000000000 +0100 +++ gcc/var-tracking.c 2012-11-19 18:59:13.638780830 +0100 @@ -9522,40 +9522,6 @@ vt_add_function_parameters (void) } } -/* Return true if INSN in the prologue initializes hard_frame_pointer_rtx. */ - -static bool -fp_setter (rtx insn) -{ - rtx pat = PATTERN (insn); - if (RTX_FRAME_RELATED_P (insn)) - { - rtx expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX); - if (expr) - pat = XEXP (expr, 0); - } - if (GET_CODE (pat) == SET) - { - if (SET_DEST (pat) != hard_frame_pointer_rtx) - return false; - } - else if (GET_CODE (pat) == PARALLEL) - { - int i; - for (i = XVECLEN (pat, 0) - 1; i >= 0; i--) - if (GET_CODE (XVECEXP (pat, 0, i)) == SET - && SET_DEST (XVECEXP (pat, 0, i)) == hard_frame_pointer_rtx) - break; - if (i < 0) - return false; - } - else - return false; - if (find_reg_note (insn, REG_CFA_RESTORE, hard_frame_pointer_rtx)) - return false; - return true; -} - /* Initialize cfa_base_rtx, create a preserved VALUE for it and ensure it isn't flushed during cselib_reset_table. Can be called only if frame_pointer_rtx resp. arg_pointer_rtx @@ -9860,7 +9826,7 @@ vt_initialize (void) if (fp_cfa_offset != -1 && hard_frame_pointer_adjustment == -1 && RTX_FRAME_RELATED_P (insn) - && fp_setter (insn)) + && fp_setter_insn (insn)) { vt_init_cfa_base (); hard_frame_pointer_adjustment = fp_cfa_offset; --- gcc/testsuite/gcc.dg/pr54921.c.jj 2012-11-19 18:52:50.016031798 +0100 +++ gcc/testsuite/gcc.dg/pr54921.c 2012-11-19 18:52:50.016031798 +0100 @@ -0,0 +1,32 @@ +/* PR rtl-optimization/54921 */ +/* { dg-do run } */ +/* { dg-options "-Os -fno-omit-frame-pointer -fsched2-use-superblocks -ftree-slp-vectorize" } */ +/* { dg-additional-options "-fstack-protector" { target fstack_protector } } */ + +struct A +{ + int a; + char b[32]; +} a, b; + +__attribute__((noinline, noclone)) +struct A +bar (int x) +{ + struct A r; + static int n; + r.a = ++n; + __builtin_memset (r.b, 0, sizeof (r.b)); + r.b[0] = x; + return r; +} + +int +main () +{ + a = bar (3); + b = bar (4); + if (a.a != 1 || a.b[0] != 3 || b.a != 2 || b.b[0] != 4) + __builtin_abort (); + return 0; +} Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Invalidate in cselib sp after processing frame_pointer_needed fp setter (PR rtl-optimization/54921, take 2) 2012-11-19 20:40 ` [PATCH] Invalidate in cselib sp after processing frame_pointer_needed fp setter (PR rtl-optimization/54921, take 2) Jakub Jelinek @ 2012-11-19 21:42 ` Richard Henderson 2012-11-19 21:45 ` Jakub Jelinek 0 siblings, 1 reply; 10+ messages in thread From: Richard Henderson @ 2012-11-19 21:42 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, Jeff Law On 11/19/2012 12:39 PM, Jakub Jelinek wrote: > + if (reload_completed > + && frame_pointer_needed > + && RTX_FRAME_RELATED_P (insn) > + && fp_setter_insn (insn)) > + cselib_invalidate_rtx (stack_pointer_rtx); ... > if (fp_cfa_offset != -1 > && hard_frame_pointer_adjustment == -1 > && RTX_FRAME_RELATED_P (insn) > - && fp_setter (insn)) > + && fp_setter_insn (insn)) I thought we talked about moving the RTX_FRAME_RELATED_P test inside the function? Since it's already there anyway... r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Invalidate in cselib sp after processing frame_pointer_needed fp setter (PR rtl-optimization/54921, take 2) 2012-11-19 21:42 ` Richard Henderson @ 2012-11-19 21:45 ` Jakub Jelinek 2012-11-19 21:58 ` Richard Henderson 0 siblings, 1 reply; 10+ messages in thread From: Jakub Jelinek @ 2012-11-19 21:45 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches, Jeff Law On Mon, Nov 19, 2012 at 01:42:06PM -0800, Richard Henderson wrote: > On 11/19/2012 12:39 PM, Jakub Jelinek wrote: > > + if (reload_completed > > + && frame_pointer_needed > > + && RTX_FRAME_RELATED_P (insn) > > + && fp_setter_insn (insn)) > > + cselib_invalidate_rtx (stack_pointer_rtx); > ... > > if (fp_cfa_offset != -1 > > && hard_frame_pointer_adjustment == -1 > > && RTX_FRAME_RELATED_P (insn) > > - && fp_setter (insn)) > > + && fp_setter_insn (insn)) > > I thought we talked about moving the RTX_FRAME_RELATED_P test inside > the function? Since it's already there anyway... Ah, forgot to remove it in the callers. Will do. Is it ok with that change? Jakub ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Invalidate in cselib sp after processing frame_pointer_needed fp setter (PR rtl-optimization/54921, take 2) 2012-11-19 21:45 ` Jakub Jelinek @ 2012-11-19 21:58 ` Richard Henderson 0 siblings, 0 replies; 10+ messages in thread From: Richard Henderson @ 2012-11-19 21:58 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, Jeff Law On 11/19/2012 01:45 PM, Jakub Jelinek wrote: > Ah, forgot to remove it in the callers. Will do. Is it ok with that change? Yes. r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-11-19 21:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-16 9:10 Patch ping Jakub Jelinek 2012-11-17 19:12 ` Richard Henderson 2012-11-17 19:16 ` Richard Henderson 2012-11-17 20:04 ` Richard Henderson 2012-11-19 7:53 ` Jakub Jelinek 2012-11-19 16:56 ` Richard Henderson 2012-11-19 20:40 ` [PATCH] Invalidate in cselib sp after processing frame_pointer_needed fp setter (PR rtl-optimization/54921, take 2) Jakub Jelinek 2012-11-19 21:42 ` Richard Henderson 2012-11-19 21:45 ` Jakub Jelinek 2012-11-19 21:58 ` Richard Henderson
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).