Message ID | 20131129215630.GG892@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
Jakub Jelinek <jakub@redhat.com> wrote: >Hi! > >The following testcase ICEs because expand_cond_expr_using_cmove >calls emit_conditional_move (which calls do_pending_stack_adjust >under some circumstances), but when that fails, just removes all the >insns generated by emit_conditional_move (and perhaps some earlier ones >too), thus it removes also the stack adjustment. > >Apparently 2 similar places were fixing it by just calling >do_pending_stack_adjust () first just in case, some other places >had (most likely) the same bug as this function. > >Rather than adding do_pending_stack_adjust () in all the places, >especially >when it isn't clear whether emit_conditional_move will be called at all >and >whether it will actually do do_pending_stack_adjust (), I chose to add >two new functions to save/restore the pending stack adjustment state, >so that when instruction sequence is thrown away (either by doing >start_sequence/end_sequence around it and not emitting it, or >delete_insns_since) the state can be restored, and have changed all the >places that IMHO need it for emit_conditional_move. > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for >trunk/4.8? The idea is good but I'd like to see a struct rather than an array for the storage. Thanks, Richard. >2013-11-29 Jakub Jelinek <jakub@redhat.com> > > PR target/58864 > * dojump.c (save_pending_stack_adjust, restore_pending_stack_adjust): > New functions. > * expr.h (save_pending_stack_adjust, restore_pending_stack_adjust): > New prototypes. > * expr.c (expand_cond_expr_using_cmove): Use it. > (expand_expr_real_2): Use it instead of unconditional > do_pending_stack_adjust. > * optabs.c (expand_doubleword_shift): Use it. > * expmed.c (expand_sdiv_pow2): Use it instead of unconditional > do_pending_stack_adjust. > (emit_store_flag): Use it. > > * g++.dg/opt/pr58864.C: New test. > >--- gcc/expr.c.jj 2013-11-27 18:02:46.000000000 +0100 >+++ gcc/expr.c 2013-11-29 14:35:12.234808484 +0100 >@@ -7951,6 +7951,9 @@ expand_cond_expr_using_cmove (tree treeo > else > temp = assign_temp (type, 0, 1); > >+ int save[2]; >+ save_pending_stack_adjust (save); >+ > start_sequence (); > expand_operands (treeop1, treeop2, > temp, &op1, &op2, EXPAND_NORMAL); >@@ -8009,6 +8012,7 @@ expand_cond_expr_using_cmove (tree treeo > /* Otherwise discard the sequence and fall back to code with > branches. */ > end_sequence (); >+ restore_pending_stack_adjust (save); > #endif > return NULL_RTX; > } >@@ -8789,12 +8793,9 @@ expand_expr_real_2 (sepops ops, rtx targ > if (can_conditionally_move_p (mode)) > { > rtx insn; >+ int save[2]; > >- /* ??? Same problem as in expmed.c: emit_conditional_move >- forces a stack adjustment via compare_from_rtx, and we >- lose the stack adjustment if the sequence we are about >- to create is discarded. */ >- do_pending_stack_adjust (); >+ save_pending_stack_adjust (save); > > start_sequence (); > >@@ -8817,6 +8818,7 @@ expand_expr_real_2 (sepops ops, rtx targ > /* Otherwise discard the sequence and fall back to code with > branches. */ > end_sequence (); >+ restore_pending_stack_adjust (save); > } > #endif > if (target != op0) >--- gcc/optabs.c.jj 2013-11-19 21:56:22.000000000 +0100 >+++ gcc/optabs.c 2013-11-29 14:39:15.963513835 +0100 >@@ -1079,17 +1079,20 @@ expand_doubleword_shift (enum machine_mo > > #ifdef HAVE_conditional_move > /* Try using conditional moves to generate straight-line code. */ >- { >- rtx start = get_last_insn (); >- if (expand_doubleword_shift_condmove (op1_mode, binoptab, >- cmp_code, cmp1, cmp2, >- outof_input, into_input, >- op1, superword_op1, >- outof_target, into_target, >- unsignedp, methods, shift_mask)) >- return true; >- delete_insns_since (start); >- } >+ int save[2]; >+ >+ save_pending_stack_adjust (save); >+ >+ rtx start = get_last_insn (); >+ if (expand_doubleword_shift_condmove (op1_mode, binoptab, >+ cmp_code, cmp1, cmp2, >+ outof_input, into_input, >+ op1, superword_op1, >+ outof_target, into_target, >+ unsignedp, methods, shift_mask)) >+ return true; >+ delete_insns_since (start); >+ restore_pending_stack_adjust (save); > #endif > >/* As a last resort, use branches to select the correct alternative. >*/ >--- gcc/dojump.c.jj 2013-11-19 21:56:27.000000000 +0100 >+++ gcc/dojump.c 2013-11-29 14:35:35.088685749 +0100 >@@ -96,6 +96,29 @@ do_pending_stack_adjust (void) > pending_stack_adjust = 0; > } > } >+ >+/* Remember pending_stack_adjust/stack_pointer_delta. >+ To be used around code that may call do_pending_stack_adjust (), >+ but the generated code could be discarded e.g. using >delete_insns_since. */ >+ >+void >+save_pending_stack_adjust (int save[2]) >+{ >+ save[0] = pending_stack_adjust; >+ save[1] = stack_pointer_delta; >+} >+ >+/* Restore the saved pending_stack_adjust/stack_pointer_delta. */ >+ >+void >+restore_pending_stack_adjust (int save[2]) >+{ >+ if (inhibit_defer_pop == 0) >+ { >+ pending_stack_adjust = save[0]; >+ stack_pointer_delta = save[1]; >+ } >+} > > > /* Expand conditional expressions. */ > >--- gcc/expr.h.jj 2013-11-19 21:56:27.000000000 +0100 >+++ gcc/expr.h 2013-11-29 14:35:23.438747999 +0100 >@@ -473,6 +473,16 @@ extern void clear_pending_stack_adjust ( >/* Pop any previously-pushed arguments that have not been popped yet. >*/ > extern void do_pending_stack_adjust (void); > >+/* Remember pending_stack_adjust/stack_pointer_delta. >+ To be used around code that may call do_pending_stack_adjust (), >+ but the generated code could be discarded e.g. using >delete_insns_since. */ >+ >+extern void save_pending_stack_adjust (int [2]); >+ >+/* Restore the saved pending_stack_adjust/stack_pointer_delta. */ >+ >+extern void restore_pending_stack_adjust (int [2]); >+ > /* Return the tree node and offset if a given argument corresponds to > a string constant. */ > extern tree string_constant (tree, tree *); >--- gcc/expmed.c.jj 2013-11-19 21:56:36.000000000 +0100 >+++ gcc/expmed.c 2013-11-29 14:41:16.887872205 +0100 >@@ -3735,11 +3735,9 @@ expand_sdiv_pow2 (enum machine_mode mode > >= 2) > { > rtx temp2; >+ int save[2]; > >- /* ??? emit_conditional_move forces a stack adjustment via >- compare_from_rtx so, if the sequence is discarded, it will >- be lost. Do it now instead. */ >- do_pending_stack_adjust (); >+ save_pending_stack_adjust (save); > > start_sequence (); > temp2 = copy_to_mode_reg (mode, op0); >@@ -3758,6 +3756,7 @@ expand_sdiv_pow2 (enum machine_mode mode > return expand_shift (RSHIFT_EXPR, mode, temp2, logd, NULL_RTX, 0); > } > end_sequence (); >+ restore_pending_stack_adjust (save); > } > #endif > >@@ -5508,6 +5507,9 @@ emit_store_flag (rtx target, enum rtx_co > if (tem == 0) > return 0; > >+ int save[2]; >+ save_pending_stack_adjust (save); >+ > if (and_them) > tem = emit_conditional_move (target, code, op0, op1, mode, > tem, const0_rtx, GET_MODE (tem), 0); >@@ -5516,7 +5518,10 @@ emit_store_flag (rtx target, enum rtx_co > trueval, tem, GET_MODE (tem), 0); > > if (tem == 0) >- delete_insns_since (last); >+ { >+ delete_insns_since (last); >+ restore_pending_stack_adjust (save); >+ } > return tem; > #else > return 0; >--- gcc/testsuite/g++.dg/opt/pr58864.C.jj 2013-11-29 13:49:24.450415660 >+0100 >+++ gcc/testsuite/g++.dg/opt/pr58864.C 2013-11-29 13:47:47.000000000 >+0100 >@@ -0,0 +1,21 @@ >+// PR target/58864 >+// { dg-do compile } >+// { dg-options "-Os" } >+// { dg-additional-options "-march=i686" { target { { i?86-*-* >x86_64-*-* } && ia32 } } } >+ >+struct A { A (); ~A (); }; >+struct B { B (); }; >+ >+float d, e; >+ >+void >+foo () >+{ >+ A a; >+ float c = d; >+ while (1) >+ { >+ B b; >+ e = c ? -c : 0; >+ } >+} > > Jakub
> Rather than adding do_pending_stack_adjust () in all the places, especially > when it isn't clear whether emit_conditional_move will be called at all and > whether it will actually do do_pending_stack_adjust (), I chose to add > two new functions to save/restore the pending stack adjustment state, > so that when instruction sequence is thrown away (either by doing > start_sequence/end_sequence around it and not emitting it, or > delete_insns_since) the state can be restored, and have changed all the > places that IMHO need it for emit_conditional_move. Why not do it in emit_conditional_move directly then? The code thinks it's clever to do: do_pending_stack_adjust (); last = get_last_insn (); prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), GET_CODE (comparison), NULL_RTX, unsignedp, OPTAB_WIDEN, &comparison, &cmode); [...] delete_insns_since (last); return NULL_RTX; but apparently not, so why not delete the stack adjustment as well and restore the state afterwards?
--- gcc/expr.c.jj 2013-11-27 18:02:46.000000000 +0100 +++ gcc/expr.c 2013-11-29 14:35:12.234808484 +0100 @@ -7951,6 +7951,9 @@ expand_cond_expr_using_cmove (tree treeo else temp = assign_temp (type, 0, 1); + int save[2]; + save_pending_stack_adjust (save); + start_sequence (); expand_operands (treeop1, treeop2, temp, &op1, &op2, EXPAND_NORMAL); @@ -8009,6 +8012,7 @@ expand_cond_expr_using_cmove (tree treeo /* Otherwise discard the sequence and fall back to code with branches. */ end_sequence (); + restore_pending_stack_adjust (save); #endif return NULL_RTX; } @@ -8789,12 +8793,9 @@ expand_expr_real_2 (sepops ops, rtx targ if (can_conditionally_move_p (mode)) { rtx insn; + int save[2]; - /* ??? Same problem as in expmed.c: emit_conditional_move - forces a stack adjustment via compare_from_rtx, and we - lose the stack adjustment if the sequence we are about - to create is discarded. */ - do_pending_stack_adjust (); + save_pending_stack_adjust (save); start_sequence (); @@ -8817,6 +8818,7 @@ expand_expr_real_2 (sepops ops, rtx targ /* Otherwise discard the sequence and fall back to code with branches. */ end_sequence (); + restore_pending_stack_adjust (save); } #endif if (target != op0) --- gcc/optabs.c.jj 2013-11-19 21:56:22.000000000 +0100 +++ gcc/optabs.c 2013-11-29 14:39:15.963513835 +0100 @@ -1079,17 +1079,20 @@ expand_doubleword_shift (enum machine_mo #ifdef HAVE_conditional_move /* Try using conditional moves to generate straight-line code. */ - { - rtx start = get_last_insn (); - if (expand_doubleword_shift_condmove (op1_mode, binoptab, - cmp_code, cmp1, cmp2, - outof_input, into_input, - op1, superword_op1, - outof_target, into_target, - unsignedp, methods, shift_mask)) - return true; - delete_insns_since (start); - } + int save[2]; + + save_pending_stack_adjust (save); + + rtx start = get_last_insn (); + if (expand_doubleword_shift_condmove (op1_mode, binoptab, + cmp_code, cmp1, cmp2, + outof_input, into_input, + op1, superword_op1, + outof_target, into_target, + unsignedp, methods, shift_mask)) + return true; + delete_insns_since (start); + restore_pending_stack_adjust (save); #endif /* As a last resort, use branches to select the correct alternative. */ --- gcc/dojump.c.jj 2013-11-19 21:56:27.000000000 +0100 +++ gcc/dojump.c 2013-11-29 14:35:35.088685749 +0100 @@ -96,6 +96,29 @@ do_pending_stack_adjust (void) pending_stack_adjust = 0; } } + +/* Remember pending_stack_adjust/stack_pointer_delta. + To be used around code that may call do_pending_stack_adjust (), + but the generated code could be discarded e.g. using delete_insns_since. */ + +void +save_pending_stack_adjust (int save[2]) +{ + save[0] = pending_stack_adjust; + save[1] = stack_pointer_delta; +} + +/* Restore the saved pending_stack_adjust/stack_pointer_delta. */ + +void +restore_pending_stack_adjust (int save[2]) +{ + if (inhibit_defer_pop == 0) + { + pending_stack_adjust = save[0]; + stack_pointer_delta = save[1]; + } +} /* Expand conditional expressions. */ --- gcc/expr.h.jj 2013-11-19 21:56:27.000000000 +0100 +++ gcc/expr.h 2013-11-29 14:35:23.438747999 +0100 @@ -473,6 +473,16 @@ extern void clear_pending_stack_adjust ( /* Pop any previously-pushed arguments that have not been popped yet. */ extern void do_pending_stack_adjust (void); +/* Remember pending_stack_adjust/stack_pointer_delta. + To be used around code that may call do_pending_stack_adjust (), + but the generated code could be discarded e.g. using delete_insns_since. */ + +extern void save_pending_stack_adjust (int [2]); + +/* Restore the saved pending_stack_adjust/stack_pointer_delta. */ + +extern void restore_pending_stack_adjust (int [2]); + /* Return the tree node and offset if a given argument corresponds to a string constant. */ extern tree string_constant (tree, tree *); --- gcc/expmed.c.jj 2013-11-19 21:56:36.000000000 +0100 +++ gcc/expmed.c 2013-11-29 14:41:16.887872205 +0100 @@ -3735,11 +3735,9 @@ expand_sdiv_pow2 (enum machine_mode mode >= 2) { rtx temp2; + int save[2]; - /* ??? emit_conditional_move forces a stack adjustment via - compare_from_rtx so, if the sequence is discarded, it will - be lost. Do it now instead. */ - do_pending_stack_adjust (); + save_pending_stack_adjust (save); start_sequence (); temp2 = copy_to_mode_reg (mode, op0); @@ -3758,6 +3756,7 @@ expand_sdiv_pow2 (enum machine_mode mode return expand_shift (RSHIFT_EXPR, mode, temp2, logd, NULL_RTX, 0); } end_sequence (); + restore_pending_stack_adjust (save); } #endif @@ -5508,6 +5507,9 @@ emit_store_flag (rtx target, enum rtx_co if (tem == 0) return 0; + int save[2]; + save_pending_stack_adjust (save); + if (and_them) tem = emit_conditional_move (target, code, op0, op1, mode, tem, const0_rtx, GET_MODE (tem), 0); @@ -5516,7 +5518,10 @@ emit_store_flag (rtx target, enum rtx_co trueval, tem, GET_MODE (tem), 0); if (tem == 0) - delete_insns_since (last); + { + delete_insns_since (last); + restore_pending_stack_adjust (save); + } return tem; #else return 0; --- gcc/testsuite/g++.dg/opt/pr58864.C.jj 2013-11-29 13:49:24.450415660 +0100 +++ gcc/testsuite/g++.dg/opt/pr58864.C 2013-11-29 13:47:47.000000000 +0100 @@ -0,0 +1,21 @@ +// PR target/58864 +// { dg-do compile } +// { dg-options "-Os" } +// { dg-additional-options "-march=i686" { target { { i?86-*-* x86_64-*-* } && ia32 } } } + +struct A { A (); ~A (); }; +struct B { B (); }; + +float d, e; + +void +foo () +{ + A a; + float c = d; + while (1) + { + B b; + e = c ? -c : 0; + } +}