* PR/32004, tree-ssa caused in/out asm constraints to often need reloads @ 2007-07-05 10:54 Paolo Bonzini [not found] ` <84fc9c000707050401h46157b0fye4b32c3e6b1c8063@mail.gmail.com> ` (2 more replies) 0 siblings, 3 replies; 25+ messages in thread From: Paolo Bonzini @ 2007-07-05 10:54 UTC (permalink / raw) To: GCC Patches; +Cc: Mark Mitchell The cause and solution of the problem are well explained in the PR trail and in the comments to the code. Bootstrapped/regtested i686-pc-linux-gnu with no new failure and fixing pr21291.c, ok for 4.2/4.3? Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <84fc9c000707050401h46157b0fye4b32c3e6b1c8063@mail.gmail.com>]
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads [not found] ` <84fc9c000707050401h46157b0fye4b32c3e6b1c8063@mail.gmail.com> @ 2007-07-05 11:48 ` Paolo Bonzini 0 siblings, 0 replies; 25+ messages in thread From: Paolo Bonzini @ 2007-07-05 11:48 UTC (permalink / raw) To: Richard Guenther, GCC Patches [-- Attachment #1: Type: text/plain, Size: 462 bytes --] Richard Guenther wrote: > On 7/5/07, Paolo Bonzini <paolo.bonzini@lu.unisi.ch> wrote: >> The cause and solution of the problem are well explained in the PR trail >> and in the comments to the code. >> >> Bootstrapped/regtested i686-pc-linux-gnu with no new failure and fixing >> pr21291.c, ok for 4.2/4.3? > > -ENOPATCH diffstat < /Users/bonzinip/cvs/pr32004.patch 5 files changed, 140 insertions(+) Note no deletions. This is 100% a new feature :-) Paolo [-- Attachment #2: pr32004.patch --] [-- Type: text/plain, Size: 7252 bytes --] 2007-07-04 Paolo Bonzini <bonzini@gnu.org> * function.c (match_asm_constraints_1, rest_of_match_asm_constraints, pass_match_asm_constraints): New. * passes.c (init_optimization_passes): Add new pass. * stmt.c (expand_asm_operands): Set cfun->has_asm_statement. * function.h (struct function): Add has_asm_statement bit. (current_function_has_asm_statement): New. * tree-pass.h (pass_match_asm_constraints): New. Index: function.c =================================================================== --- function.c (revision 126190) +++ function.c (working copy) @@ -5504,6 +5504,139 @@ struct tree_opt_pass pass_thread_prologu TODO_ggc_collect, /* todo_flags_finish */ 'w' /* letter */ }; +\f + +/* This mini-pass fixes fall-out from SSA in asm statements that have + in-out constraints. Say you start with + + orig = inout; + asm ("": "+mr" (inout)); + use (orig); + + which is transformed very early to use explicit output and match operands: + + orig = inout; + asm ("": "=mr" (inout) : "0" (inout)); + use (orig); + + Or, after SSA and copyprop, + + asm ("": "=mr" (inout_2) : "0" (inout_1)); + use (inout_1); + + Clearly inout_2 and inout_1 can't be coalesced easily anymore, as + they represent two separate values, so they will get different pseudo + registers during expansion. Then, since the two operands need to match + per the constraints, but use different pseudo registers, reload can + only register a reload for these operands. But reloads can only be + satisfied by hardregs, not by memory, so we need a register for this + reload, just because we are presented with non-matching operands. + So, even though we allow memory for this operand, no memory can be + used for it, just because the two operands don't match. This can + cause reload failures on register-starved targets. + + So it's a symptom of reload not being able to use memory for reloads + or, alternatively it's also a symptom of both operands not coming into + reload as matching (in which case the pseudo could go to memory just + fine, as the alternative allows it, and no reload would be necessary). + We fix the latter problem here, by transforming + + asm ("": "=mr" (inout_2) : "0" (inout_1)); + + back to + + inout_2 = inout_1; + asm ("": "=mr" (inout_2) : "0" (inout_2)); */ + +static void +match_asm_constraints_1 (rtx insn, rtx *sets, int noutputs) +{ + int i; + bool changed = false; + rtx op = SET_SRC (sets[0]); + int ninputs = ASM_OPERANDS_INPUT_LENGTH (op); + rtvec inputs = ASM_OPERANDS_INPUT_VEC (op); + + for (i = 0; i < ninputs; i++) + { + rtx input, output, insns; + const char *constraint = ASM_OPERANDS_INPUT_CONSTRAINT (op, i); + char *end; + int match; + + match = strtoul (constraint, &end, 10); + if (end == constraint) + continue; + + gcc_assert (match < noutputs); + output = SET_DEST (sets[match]); + input = RTVEC_ELT (inputs, i); + if (rtx_equal_p (output, input)) + continue; + + start_sequence (); + emit_move_insn (output, input); + RTVEC_ELT (inputs, i) = copy_rtx (output); + insns = get_insns (); + end_sequence (); + + emit_insn_before (insns, insn); + changed = true; + } + + if (changed) + df_insn_rescan (insn); +} + +static unsigned +rest_of_match_asm_constraints (void) +{ + basic_block bb; + rtx insn, pat; + + if (!cfun->has_asm_statement) + return 0; + + df_set_flags (DF_DEFER_INSN_RESCAN); + FOR_EACH_BB (bb) + { + FOR_BB_INSNS (bb, insn) + { + if (!INSN_P (insn)) + continue; + + pat = PATTERN (insn); + if (GET_CODE (pat) == PARALLEL + && GET_CODE (XVECEXP (pat, 0, 0)) == SET + && GET_CODE (SET_SRC (XVECEXP (pat, 0, 0))) == ASM_OPERANDS) + match_asm_constraints_1 (insn, + &XVECEXP (pat, 0, 0), XVECLEN (pat, 0)); + + else if (GET_CODE (pat) == SET + && GET_CODE (SET_SRC (pat)) == ASM_OPERANDS) + match_asm_constraints_1 (insn, &pat, 1); + } + } + + return TODO_df_finish; +} + +struct tree_opt_pass pass_match_asm_constraints = +{ + "asmcons", /* name */ + NULL, /* gate */ + rest_of_match_asm_constraints, /* execute */ + NULL, /* sub */ + NULL, /* next */ + 0, /* static_pass_number */ + 0, /* tv_id */ + 0, /* properties_required */ + 0, /* properties_provided */ + 0, /* properties_destroyed */ + 0, /* todo_flags_start */ + TODO_dump_func, /* todo_flags_finish */ + 0 /* letter */ +}; #include "gt-function.h" Index: passes.c =================================================================== --- passes.c (revision 126190) +++ passes.c (working copy) @@ -740,6 +740,7 @@ init_optimization_passes (void) NEXT_PASS (pass_stack_ptr_mod); NEXT_PASS (pass_mode_switching); NEXT_PASS (pass_see); + NEXT_PASS (pass_match_asm_constraints); NEXT_PASS (pass_sms); NEXT_PASS (pass_sched); NEXT_PASS (pass_subregs_of_mode_init); Index: stmt.c =================================================================== --- stmt.c (revision 126190) +++ stmt.c (working copy) @@ -1078,6 +1078,7 @@ expand_asm_operands (tree string, tree o if (real_output_rtx[i]) emit_move_insn (real_output_rtx[i], output_rtx[i]); + cfun->has_asm_statement = 1; free_temp_slots (); } Index: function.h =================================================================== --- function.h (revision 126190) +++ function.h (working copy) @@ -414,6 +414,9 @@ struct function GTY(()) /* Nonzero if function being compiled has nonlocal gotos to parent function. */ unsigned int has_nonlocal_goto : 1; + + /* Nonzero if function being compiled has an asm statement. */ + unsigned int has_asm_statement : 1; /* Nonzero if the current function is a thunk, i.e., a lightweight function implemented by the output_mi_thunk hook) that just @@ -517,6 +520,7 @@ extern int trampolines_created; #define current_function_has_nonlocal_label (cfun->has_nonlocal_label) #define current_function_calls_unwind_init (cfun->calls_unwind_init) #define current_function_has_nonlocal_goto (cfun->has_nonlocal_goto) +#define current_function_has_asm_statement (cfun->has_asm_statement) #define return_label (cfun->x_return_label) #define naked_return_label (cfun->x_naked_return_label) Index: tree-pass.h =================================================================== --- tree-pass.h (revision 126190) +++ tree-pass.h (working copy) @@ -390,6 +390,7 @@ extern struct tree_opt_pass pass_initial extern struct tree_opt_pass pass_combine; extern struct tree_opt_pass pass_if_after_combine; extern struct tree_opt_pass pass_partition_blocks; +extern struct tree_opt_pass pass_match_asm_constraints; extern struct tree_opt_pass pass_regmove; extern struct tree_opt_pass pass_split_all_insns; extern struct tree_opt_pass pass_lower_subreg2; ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-05 10:54 PR/32004, tree-ssa caused in/out asm constraints to often need reloads Paolo Bonzini [not found] ` <84fc9c000707050401h46157b0fye4b32c3e6b1c8063@mail.gmail.com> @ 2007-07-05 18:34 ` Mark Mitchell 2007-07-05 19:39 ` Paolo Bonzini 2007-07-05 20:15 ` Paolo Bonzini 2007-07-07 16:29 ` Eric Botcazou 2 siblings, 2 replies; 25+ messages in thread From: Mark Mitchell @ 2007-07-05 18:34 UTC (permalink / raw) To: bonzini; +Cc: GCC Patches Paolo Bonzini wrote: > The cause and solution of the problem are well explained in the PR trail > and in the comments to the code. > > Bootstrapped/regtested i686-pc-linux-gnu with no new failure and fixing > pr21291.c, ok for 4.2/4.3? I have a couple of minor comments: + if (GET_CODE (pat) == PARALLEL + && GET_CODE (XVECEXP (pat, 0, 0)) == SET + && GET_CODE (SET_SRC (XVECEXP (pat, 0, 0))) == ASM_OPERANDS) + match_asm_constraints_1 (insn, + &XVECEXP (pat, 0, 0), XVECLEN (pat, 0)); + + else if (GET_CODE (pat) == SET + && GET_CODE (SET_SRC (pat)) == ASM_OPERANDS) + match_asm_constraints_1 (insn, &pat, 1); could be more tidily expressed as: if (GET_CODE (pat) == PARALLEL) pat = XVECEXP (pat, 0, 0); if (GET_CODE (pat) == SET && ...) ... Also: + int match; + match = strtoul (constraint, &end, 10); it seems like it would be better just to declare "match" to be "unsigned long". Does your patch handle matching constraints given by "[NAME]"? Other than that, it looks good. -- Mark Mitchell CodeSourcery mark@codesourcery.com (650) 331-3385 x713 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-05 18:34 ` Mark Mitchell @ 2007-07-05 19:39 ` Paolo Bonzini 2007-07-05 20:24 ` Mark Mitchell 2007-07-05 20:15 ` Paolo Bonzini 1 sibling, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2007-07-05 19:39 UTC (permalink / raw) To: Mark Mitchell; +Cc: GCC Patches > if (GET_CODE (pat) == PARALLEL) > pat = XVECEXP (pat, 0, 0); > if (GET_CODE (pat) == SET > && ...) > ... Note I would still have to store XVECLEN somewhere, but I'll rewrite the test along these lines. > Also: > > + int match; > + match = strtoul (constraint, &end, 10); > > it seems like it would be better just to declare "match" to be "unsigned > long". Yes. Named constraints are handled in stmt.c. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-05 19:39 ` Paolo Bonzini @ 2007-07-05 20:24 ` Mark Mitchell 0 siblings, 0 replies; 25+ messages in thread From: Mark Mitchell @ 2007-07-05 20:24 UTC (permalink / raw) To: bonzini; +Cc: GCC Patches Paolo Bonzini wrote: >> if (GET_CODE (pat) == PARALLEL) >> pat = XVECEXP (pat, 0, 0); >> if (GET_CODE (pat) == SET >> && ...) >> ... > > Note I would still have to store XVECLEN somewhere, but I'll rewrite the > test along these lines. Thanks. >> it seems like it would be better just to declare "match" to be "unsigned >> long". > > Yes. > > Named constraints are handled in stmt.c. Good, thanks for checking. Patch OK with those changes. -- Mark Mitchell CodeSourcery mark@codesourcery.com (650) 331-3385 x713 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-05 18:34 ` Mark Mitchell 2007-07-05 19:39 ` Paolo Bonzini @ 2007-07-05 20:15 ` Paolo Bonzini 1 sibling, 0 replies; 25+ messages in thread From: Paolo Bonzini @ 2007-07-05 20:15 UTC (permalink / raw) To: Mark Mitchell; +Cc: GCC Patches [-- Attachment #1: Type: text/plain, Size: 310 bytes --] Mark Mitchell wrote: > Paolo Bonzini wrote: >> The cause and solution of the problem are well explained in the PR trail >> and in the comments to the code. >> >> Bootstrapped/regtested i686-pc-linux-gnu with no new failure and fixing >> pr21291.c, ok for 4.2/4.3? Here is what I committed to mainline. Paolo [-- Attachment #2: pr32004.patch --] [-- Type: text/plain, Size: 7357 bytes --] 2007-07-04 Paolo Bonzini <bonzini@gnu.org> * function.c (match_asm_constraints_1, rest_of_match_asm_constraints, pass_match_asm_constraints): New. * passes.c (init_optimization_passes): Add new pass. * stmt.c (expand_asm_operands): Set cfun->has_asm_statement. * function.h (struct function): Add has_asm_statement bit. (current_function_has_asm_statement): New. * tree-pass.h (pass_match_asm_constraints): New. Index: function.c =================================================================== --- function.c (revision 126190) +++ function.c (working copy) @@ -5504,6 +5504,143 @@ struct tree_opt_pass pass_thread_prologu TODO_ggc_collect, /* todo_flags_finish */ 'w' /* letter */ }; +\f + +/* This mini-pass fixes fall-out from SSA in asm statements that have + in-out constraints. Say you start with + + orig = inout; + asm ("": "+mr" (inout)); + use (orig); + + which is transformed very early to use explicit output and match operands: + + orig = inout; + asm ("": "=mr" (inout) : "0" (inout)); + use (orig); + + Or, after SSA and copyprop, + + asm ("": "=mr" (inout_2) : "0" (inout_1)); + use (inout_1); + + Clearly inout_2 and inout_1 can't be coalesced easily anymore, as + they represent two separate values, so they will get different pseudo + registers during expansion. Then, since the two operands need to match + per the constraints, but use different pseudo registers, reload can + only register a reload for these operands. But reloads can only be + satisfied by hardregs, not by memory, so we need a register for this + reload, just because we are presented with non-matching operands. + So, even though we allow memory for this operand, no memory can be + used for it, just because the two operands don't match. This can + cause reload failures on register-starved targets. + + So it's a symptom of reload not being able to use memory for reloads + or, alternatively it's also a symptom of both operands not coming into + reload as matching (in which case the pseudo could go to memory just + fine, as the alternative allows it, and no reload would be necessary). + We fix the latter problem here, by transforming + + asm ("": "=mr" (inout_2) : "0" (inout_1)); + + back to + + inout_2 = inout_1; + asm ("": "=mr" (inout_2) : "0" (inout_2)); */ + +static void +match_asm_constraints_1 (rtx insn, rtx *p_sets, int noutputs) +{ + int i; + bool changed = false; + rtx op = SET_SRC (p_sets[0]); + int ninputs = ASM_OPERANDS_INPUT_LENGTH (op); + rtvec inputs = ASM_OPERANDS_INPUT_VEC (op); + + for (i = 0; i < ninputs; i++) + { + rtx input, output, insns; + const char *constraint = ASM_OPERANDS_INPUT_CONSTRAINT (op, i); + char *end; + int match; + + match = strtoul (constraint, &end, 10); + if (end == constraint) + continue; + + gcc_assert (match < noutputs); + output = SET_DEST (p_sets[match]); + input = RTVEC_ELT (inputs, i); + if (rtx_equal_p (output, input) + || (GET_MODE (input) != VOIDmode + && GET_MODE (input) != GET_MODE (output))) + continue; + + start_sequence (); + emit_move_insn (copy_rtx (output), input); + RTVEC_ELT (inputs, i) = copy_rtx (output); + insns = get_insns (); + end_sequence (); + + emit_insn_before (insns, insn); + changed = true; + } + + if (changed) + df_insn_rescan (insn); +} + +static unsigned +rest_of_match_asm_constraints (void) +{ + basic_block bb; + rtx insn, pat, *p_sets; + int noutputs; + + if (!cfun->has_asm_statement) + return 0; + + df_set_flags (DF_DEFER_INSN_RESCAN); + FOR_EACH_BB (bb) + { + FOR_BB_INSNS (bb, insn) + { + if (!INSN_P (insn)) + continue; + + pat = PATTERN (insn); + if (GET_CODE (pat) == PARALLEL) + p_sets = &XVECEXP (pat, 0, 0), noutputs = XVECLEN (pat, 0); + else if (GET_CODE (pat) == SET) + p_sets = &PATTERN (insn), noutputs = 1; + else + continue; + + if (GET_CODE (*p_sets) == SET + && GET_CODE (SET_SRC (*p_sets)) == ASM_OPERANDS) + match_asm_constraints_1 (insn, p_sets, noutputs); + } + } + + return TODO_df_finish; +} + +struct tree_opt_pass pass_match_asm_constraints = +{ + "asmcons", /* name */ + NULL, /* gate */ + rest_of_match_asm_constraints, /* execute */ + NULL, /* sub */ + NULL, /* next */ + 0, /* static_pass_number */ + 0, /* tv_id */ + 0, /* properties_required */ + 0, /* properties_provided */ + 0, /* properties_destroyed */ + 0, /* todo_flags_start */ + TODO_dump_func, /* todo_flags_finish */ + 0 /* letter */ +}; #include "gt-function.h" Index: passes.c =================================================================== --- passes.c (revision 126190) +++ passes.c (working copy) @@ -740,6 +740,7 @@ init_optimization_passes (void) NEXT_PASS (pass_stack_ptr_mod); NEXT_PASS (pass_mode_switching); NEXT_PASS (pass_see); + NEXT_PASS (pass_match_asm_constraints); NEXT_PASS (pass_sms); NEXT_PASS (pass_sched); NEXT_PASS (pass_subregs_of_mode_init); Index: stmt.c =================================================================== --- stmt.c (revision 126190) +++ stmt.c (working copy) @@ -1078,6 +1078,7 @@ expand_asm_operands (tree string, tree o if (real_output_rtx[i]) emit_move_insn (real_output_rtx[i], output_rtx[i]); + cfun->has_asm_statement = 1; free_temp_slots (); } Index: function.h =================================================================== --- function.h (revision 126190) +++ function.h (working copy) @@ -414,6 +414,9 @@ struct function GTY(()) /* Nonzero if function being compiled has nonlocal gotos to parent function. */ unsigned int has_nonlocal_goto : 1; + + /* Nonzero if function being compiled has an asm statement. */ + unsigned int has_asm_statement : 1; /* Nonzero if the current function is a thunk, i.e., a lightweight function implemented by the output_mi_thunk hook) that just @@ -517,6 +520,7 @@ extern int trampolines_created; #define current_function_has_nonlocal_label (cfun->has_nonlocal_label) #define current_function_calls_unwind_init (cfun->calls_unwind_init) #define current_function_has_nonlocal_goto (cfun->has_nonlocal_goto) +#define current_function_has_asm_statement (cfun->has_asm_statement) #define return_label (cfun->x_return_label) #define naked_return_label (cfun->x_naked_return_label) Index: tree-pass.h =================================================================== --- tree-pass.h (revision 126190) +++ tree-pass.h (working copy) @@ -390,6 +390,7 @@ extern struct tree_opt_pass pass_initial extern struct tree_opt_pass pass_combine; extern struct tree_opt_pass pass_if_after_combine; extern struct tree_opt_pass pass_partition_blocks; +extern struct tree_opt_pass pass_match_asm_constraints; extern struct tree_opt_pass pass_regmove; extern struct tree_opt_pass pass_split_all_insns; extern struct tree_opt_pass pass_lower_subreg2; ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-05 10:54 PR/32004, tree-ssa caused in/out asm constraints to often need reloads Paolo Bonzini [not found] ` <84fc9c000707050401h46157b0fye4b32c3e6b1c8063@mail.gmail.com> 2007-07-05 18:34 ` Mark Mitchell @ 2007-07-07 16:29 ` Eric Botcazou 2007-07-07 18:24 ` Andrew Pinski 2 siblings, 1 reply; 25+ messages in thread From: Eric Botcazou @ 2007-07-07 16:29 UTC (permalink / raw) To: bonzini; +Cc: gcc-patches, Mark Mitchell > Bootstrapped/regtested i686-pc-linux-gnu with no new failure and fixing > pr21291.c, ok for 4.2/4.3? AFAICS you didn't request approval for the 4.1 branch, but nevertheless installed the patch on this branch too. Now I get with RTL checking on: FAIL: gcc.dg/torture/pr20314-1.c -O1 (internal compiler error) FAIL: gcc.dg/torture/pr20314-1.c -O1 (test for excess errors) FAIL: gcc.dg/torture/pr20314-1.c -O2 (internal compiler error) FAIL: gcc.dg/torture/pr20314-1.c -O2 (test for excess errors) FAIL: gcc.dg/torture/pr20314-1.c -O3 -fomit-frame-pointer (internal compiler error) FAIL: gcc.dg/torture/pr20314-1.c -O3 -fomit-frame-pointer (test for excess errors) FAIL: gcc.dg/torture/pr20314-1.c -O3 -g (internal compiler error) FAIL: gcc.dg/torture/pr20314-1.c -O3 -g (test for excess errors) FAIL: gcc.dg/torture/pr20314-1.c -Os (internal compiler error) FAIL: gcc.dg/torture/pr20314-1.c -Os (test for excess errors) Executing on host: /home/eric/gnat/gnat6_41/native32/gcc/xgcc -B/home/eric/gnat/gnat6_41/native32/gcc/ /home/eric/gnat/gnat6_41/src/gcc/testsuite/gcc.dg/torture/pr20314-1.c -O0 -fno-show-column -S -o pr20314-1.s (timeout = 300) PASS: gcc.dg/torture/pr20314-1.c -O0 (test for excess errors) Executing on host: /home/eric/gnat/gnat6_41/native32/gcc/xgcc -B/home/eric/gnat/gnat6_41/native32/gcc/ /home/eric/gnat/gnat6_41/src/gcc/testsuite/gcc.dg/torture/pr20314-1.c -O1 -fno-show-column -S -o pr20314-1.s (timeout = 300) /home/eric/gnat/gnat6_41/src/gcc/testsuite/gcc.dg/torture/pr20314-1.c: In function 'f3': /home/eric/gnat/gnat6_41/src/gcc/testsuite/gcc.dg/torture/pr20314-1.c:29: internal compiler error: RTL check: access of elt 1 of 'const_int' with last elt 0 in update_equiv_regs, at local-alloc.c:1117 Please submit a full bug report, with preprocessed source if appropriate. See <URL:mailto:report@adacore.com> for instructions. compiler exited with status 1 Please revert the patch on the 4.1 branch. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-07 16:29 ` Eric Botcazou @ 2007-07-07 18:24 ` Andrew Pinski 2007-07-07 18:30 ` Eric Botcazou 0 siblings, 1 reply; 25+ messages in thread From: Andrew Pinski @ 2007-07-07 18:24 UTC (permalink / raw) To: Eric Botcazou; +Cc: bonzini, gcc-patches, Mark Mitchell On 7/7/07, Eric Botcazou <ebotcazou@adacore.com> wrote: > /home/eric/gnat/gnat6_41/src/gcc/testsuite/gcc.dg/torture/pr20314-1.c:29: > internal compiler error: RTL check: access of elt 1 of 'const_int' with last > elt 0 in update_equiv_regs, at local-alloc.c:1117 His patch did not even touch local-alloc.c at all or anything which could have caused this directly (rtl.def). So this is a latent bug. The main reason why he applied it without any approval is based on the rule that the bug is a regression and it was approved for the trunk. -- Pinski ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-07 18:24 ` Andrew Pinski @ 2007-07-07 18:30 ` Eric Botcazou 2007-07-09 2:23 ` Paolo Bonzini 2007-07-13 9:48 ` Paolo Bonzini 0 siblings, 2 replies; 25+ messages in thread From: Eric Botcazou @ 2007-07-07 18:30 UTC (permalink / raw) To: Andrew Pinski; +Cc: bonzini, gcc-patches, Mark Mitchell > His patch did not even touch local-alloc.c at all or anything which > could have caused this directly (rtl.def). So this is a latent bug. It might well be, but did you investigate? > The main reason why he applied it without any approval is based on the > rule that the bug is a regression and it was approved for the trunk. You don't put a brand new pass on a branch that is in maintenance mode, especially if you cannot disable it, precisely because you don't know how it will interact with the rest of the compiler. If Paolo had requested approval for the 4.1 branch, I'd have opposed. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-07 18:30 ` Eric Botcazou @ 2007-07-09 2:23 ` Paolo Bonzini 2007-07-09 6:52 ` Eric Botcazou 2007-07-13 9:48 ` Paolo Bonzini 1 sibling, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2007-07-09 2:23 UTC (permalink / raw) To: Eric Botcazou; +Cc: Andrew Pinski, gcc-patches, Mark Mitchell Eric Botcazou wrote: >> His patch did not even touch local-alloc.c at all or anything which >> could have caused this directly (rtl.def). So this is a latent bug. I will look at it and may eventually revert it, but I'll note that this "brand new pass" just adds a few (set (reg) (reg)). Other people might have just dropped it as part of regmove, would that have been acceptable to you? Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 2:23 ` Paolo Bonzini @ 2007-07-09 6:52 ` Eric Botcazou 2007-07-09 12:09 ` Paolo Bonzini 2007-07-09 12:32 ` Paolo Bonzini 0 siblings, 2 replies; 25+ messages in thread From: Eric Botcazou @ 2007-07-09 6:52 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Andrew Pinski, gcc-patches, Mark Mitchell > I will look at it and may eventually revert it, but I'll note that this > "brand new pass" just adds a few (set (reg) (reg)). Please remove it from the 4.1 branch, you didn't get approval to put it there and you apparently didn't even test it there, causing the first regressions in months on the branch. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 6:52 ` Eric Botcazou @ 2007-07-09 12:09 ` Paolo Bonzini 2007-07-09 12:32 ` Paolo Bonzini 1 sibling, 0 replies; 25+ messages in thread From: Paolo Bonzini @ 2007-07-09 12:09 UTC (permalink / raw) To: Eric Botcazou; +Cc: Paolo Bonzini, Andrew Pinski, gcc-patches, Mark Mitchell Eric Botcazou wrote: >> I will look at it and may eventually revert it, but I'll note that this >> "brand new pass" just adds a few (set (reg) (reg)). > > Please remove it from the 4.1 branch, you didn't get approval to put it there > and you apparently didn't even test it there, causing the first regressions > in months on the branch. No, I just looked at the wrong log file. I'm reverting it. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 6:52 ` Eric Botcazou 2007-07-09 12:09 ` Paolo Bonzini @ 2007-07-09 12:32 ` Paolo Bonzini 2007-07-09 12:40 ` Richard Guenther 2007-07-09 13:01 ` Eric Botcazou 1 sibling, 2 replies; 25+ messages in thread From: Paolo Bonzini @ 2007-07-09 12:32 UTC (permalink / raw) To: Eric Botcazou; +Cc: Paolo Bonzini, Andrew Pinski, gcc-patches, Mark Mitchell Eric Botcazou wrote: >> I will look at it and may eventually revert it, but I'll note that this >> "brand new pass" just adds a few (set (reg) (reg)). > > Please remove it from the 4.1 branch, you didn't get approval to put it there > and you apparently didn't even test it there, causing the first regressions > in months on the branch. Actually it is fixed by this trivial patch. Ok for 4.2/4.1? Paolo Index: ../base-gcc-src/gcc/function.c =================================================================== --- ../base-gcc-src/gcc/function.c (revision 126418) +++ ../base-gcc-src/gcc/function.c (working copy) @@ -5713,7 +5713,7 @@ rest_of_match_asm_constraints (void) } update_life_info_in_dirty_blocks (UPDATE_LIFE_GLOBAL_RM_NOTES, - PROP_DEATH_NOTES); + PROP_DEATH_NOTES | PROP_REG_INFO); return 0; } ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 12:32 ` Paolo Bonzini @ 2007-07-09 12:40 ` Richard Guenther 2007-07-09 13:01 ` Eric Botcazou 1 sibling, 0 replies; 25+ messages in thread From: Richard Guenther @ 2007-07-09 12:40 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Eric Botcazou, Andrew Pinski, gcc-patches, Mark Mitchell On 7/9/07, Paolo Bonzini <bonzini@gnu.org> wrote: > Eric Botcazou wrote: > >> I will look at it and may eventually revert it, but I'll note that this > >> "brand new pass" just adds a few (set (reg) (reg)). > > > > Please remove it from the 4.1 branch, you didn't get approval to put it there > > and you apparently didn't even test it there, causing the first regressions > > in months on the branch. > > Actually it is fixed by this trivial patch. Ok for 4.2/4.1? Assuming you have tested the patch on the branches, yes. Richard. > Paolo > > Index: ../base-gcc-src/gcc/function.c > =================================================================== > --- ../base-gcc-src/gcc/function.c (revision 126418) > +++ ../base-gcc-src/gcc/function.c (working copy) > @@ -5713,7 +5713,7 @@ rest_of_match_asm_constraints (void) > } > > update_life_info_in_dirty_blocks (UPDATE_LIFE_GLOBAL_RM_NOTES, > - PROP_DEATH_NOTES); > + PROP_DEATH_NOTES | PROP_REG_INFO); > return 0; > } > > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 12:32 ` Paolo Bonzini 2007-07-09 12:40 ` Richard Guenther @ 2007-07-09 13:01 ` Eric Botcazou 2007-07-09 13:01 ` Richard Guenther 2007-07-09 13:12 ` Paolo Bonzini 1 sibling, 2 replies; 25+ messages in thread From: Eric Botcazou @ 2007-07-09 13:01 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Andrew Pinski, gcc-patches, Mark Mitchell > Actually it is fixed by this trivial patch. Ok for 4.2/4.1? This patch is *not* trivial. The RTL dataflow engine in flow.c has known quirks and we should be conservative in changing how it is invoked. Again, you didn't get approval for the 4.1 branch, please revert the patch. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 13:01 ` Eric Botcazou @ 2007-07-09 13:01 ` Richard Guenther 2007-07-09 13:36 ` Eric Botcazou 2007-07-09 13:12 ` Paolo Bonzini 1 sibling, 1 reply; 25+ messages in thread From: Richard Guenther @ 2007-07-09 13:01 UTC (permalink / raw) To: Eric Botcazou; +Cc: Paolo Bonzini, Andrew Pinski, gcc-patches, Mark Mitchell On 7/9/07, Eric Botcazou <ebotcazou@adacore.com> wrote: > > Actually it is fixed by this trivial patch. Ok for 4.2/4.1? > > This patch is *not* trivial. The RTL dataflow engine in flow.c has known > quirks and we should be conservative in changing how it is invoked. > > Again, you didn't get approval for the 4.1 branch, please revert the patch. There is no formal need of approval for porting regression fixes from the trunk to release branches. If this is good or bad is another question. Richard. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 13:01 ` Richard Guenther @ 2007-07-09 13:36 ` Eric Botcazou 0 siblings, 0 replies; 25+ messages in thread From: Eric Botcazou @ 2007-07-09 13:36 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches, Paolo Bonzini, Andrew Pinski, Mark Mitchell > There is no formal need of approval for porting regression fixes from the > trunk to release branches. If this is good or bad is another question. Well, there is at least an implicit one. Note that Paolo did explicitly request approval for the 4.2 branch, and I don't think we should be more lax for the 4.1 branch than for the 4.2 branch, quite the contrary. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 13:01 ` Eric Botcazou 2007-07-09 13:01 ` Richard Guenther @ 2007-07-09 13:12 ` Paolo Bonzini 2007-07-09 13:23 ` Andrew Pinski 2007-07-09 14:00 ` Eric Botcazou 1 sibling, 2 replies; 25+ messages in thread From: Paolo Bonzini @ 2007-07-09 13:12 UTC (permalink / raw) To: Eric Botcazou; +Cc: Paolo Bonzini, Andrew Pinski, gcc-patches, Mark Mitchell Eric Botcazou wrote: >> Actually it is fixed by this trivial patch. Ok for 4.2/4.1? > > This patch is *not* trivial. The RTL dataflow engine in flow.c has known > quirks and we should be conservative in changing how it is invoked. That's why I'm invoking in GLOBAL_RM_NOTES rather than LOCAL which ought to be enough. That's conservativeness. I do believe the patch is trivial (note I didn't say obvious) if you investigated the failure mode. The problem is that reg_equiv_regs is set when REG_N_REFS is 2 and in this case we have more than 2 after the new pass. The patch is in testing now, and the gcc directory has already been tested fully on both the 4.1 and 4.2 branches. If you want me to revert the patch on 4.1 and apply the combination in a few days, after letting it stay on 4.2 for a while, I can do that. Otherwise, I would very much prefer fixing PR21291/PR32004, which is as bad a regression as the one I introduced. Of course, I'm not meaning I'm *not* sorry to have introduced a regression. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 13:12 ` Paolo Bonzini @ 2007-07-09 13:23 ` Andrew Pinski 2007-07-09 13:45 ` Eric Botcazou 2007-07-09 15:33 ` Paolo Bonzini 2007-07-09 14:00 ` Eric Botcazou 1 sibling, 2 replies; 25+ messages in thread From: Andrew Pinski @ 2007-07-09 13:23 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Eric Botcazou, gcc-patches, Mark Mitchell On 7/9/07, Paolo Bonzini <bonzini@gnu.org> wrote: > If you want me to revert the patch on 4.1 and apply the combination in a > few days, after letting it stay on 4.2 for a while, I can do that. > Otherwise, I would very much prefer fixing PR21291/PR32004, which is as > bad a regression as the one I introduced. Of course, I'm not meaning > I'm *not* sorry to have introduced a regression. >>Now I get with RTL checking on: Just a quick note, it is not a requirement (as far as I know), that we need to turn on RTL checking to test backports of patches. Now I can see if this was an issue if the regression showed up with the default checking for that branch but the regression as far as I can tell only shows up with RTL checking turned on (which is not even on the trunk) so the policy of not requiring approval for backporting a regression fix (with a bootstrap/test) was not violated here as far as I can tell. Just a latent bug showed up because the trunk and the branches have large differences in the dataflow. -- Pinski ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 13:23 ` Andrew Pinski @ 2007-07-09 13:45 ` Eric Botcazou 2007-07-09 15:33 ` Paolo Bonzini 1 sibling, 0 replies; 25+ messages in thread From: Eric Botcazou @ 2007-07-09 13:45 UTC (permalink / raw) To: Andrew Pinski; +Cc: Paolo Bonzini, gcc-patches, Mark Mitchell > Now I can see if this was an issue if the regression showed up with the > default checking for that branch but the regression as far as I can tell > only shows up with RTL checking turned on (which is not even on the trunk) That's wrong, regressions have been introduced in release checking mode: http://gcc.gnu.org/ml/gcc-testresults/2007-07/msg00314.html http://gcc.gnu.org/ml/gcc-testresults/2007-07/msg00315.html -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 13:23 ` Andrew Pinski 2007-07-09 13:45 ` Eric Botcazou @ 2007-07-09 15:33 ` Paolo Bonzini 2007-07-09 17:25 ` Eric Botcazou 1 sibling, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2007-07-09 15:33 UTC (permalink / raw) To: Andrew Pinski; +Cc: Eric Botcazou, gcc-patches, Mark Mitchell > Just a latent bug showed up because the trunk and the branches > have large differences in the dataflow. I don't need any kind of defense because I *did* make a mistake in the testing and because this is *not* a latent bug. The backport was not as obvious as I had thought, so in retrospect what I should have done would have been to first submit a patch for 4.3 only, and then a patch for 4.2/4.1. I remember I had made the same mistake (not looking at the wrong logs, O:-) rather assuming that the same fix applied to two wildly different codebases) with a performance PR affecting 4.0 and 3.4 (I don't remember the PR number); in that case Eric also had suggested reverting on 3.4 and I did that exactly. In this case however this is a much worse bug (it is P1 for a reason, even if the testcase was not reported by a user), so I'm committing the additional fix which has been approved by Richard. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 15:33 ` Paolo Bonzini @ 2007-07-09 17:25 ` Eric Botcazou 0 siblings, 0 replies; 25+ messages in thread From: Eric Botcazou @ 2007-07-09 17:25 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Andrew Pinski, gcc-patches, Mark Mitchell > In this case however this is a much worse bug (it is P1 for a reason, > even if the testcase was not reported by a user), so I'm committing the > additional fix which has been approved by Richard. If the problem has never been reported by a user, it cannot be reasonably P1 at this point. Remember that the 4.1 branch is used by many vendors as their system compiler so we must be conservative with what we put on it. You have acknowledged that you made a mistake by not submitting a separate patch for the 4.2/4.1 branches. If you didn't make this mistake, I'd have seen the request for the 4.1 branch and opposed it; your patch being within the purview of my maintainership, there are reasonable chances that my opinion would have prevailed. So, please, correct your initial mistake and formally resubmit the patch for approval for the 4.2/4.1 branches. I'll still oppose it for the 4.1 branch so you'll need to find 2 other maintainers to overrule my position; if you fail, you'll have to revert the patch on the branch. Thanks in advance. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-09 13:12 ` Paolo Bonzini 2007-07-09 13:23 ` Andrew Pinski @ 2007-07-09 14:00 ` Eric Botcazou 1 sibling, 0 replies; 25+ messages in thread From: Eric Botcazou @ 2007-07-09 14:00 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Andrew Pinski, gcc-patches, Mark Mitchell > I do believe the patch is trivial (note I didn't say obvious) if you > investigated the failure mode. I'd have said the opposite (it's an obvious fix but not a trivial change, because of the quirks of the dataflow engine) but I'm not a native speaker. > Otherwise, I would very much prefer fixing PR21291/PR32004, which is as > bad a regression as the one I introduced. Of course, I'm not meaning > I'm *not* sorry to have introduced a regression. But PR21291/PR32004 was reported against 4.3 only! A testcase was later found that fails with all 4.x compilers, but AFAICS it was not reported by a user. So I still think that this patch should not have been put on the 4.1 branch. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-07 18:30 ` Eric Botcazou 2007-07-09 2:23 ` Paolo Bonzini @ 2007-07-13 9:48 ` Paolo Bonzini 2007-07-13 13:42 ` Eric Botcazou 1 sibling, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2007-07-13 9:48 UTC (permalink / raw) To: Eric Botcazou; +Cc: gcc-patches > You don't put a brand new pass on a branch that is in maintenance mod I've reverted it now. Sorry for the useless arguing. Paolo ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads 2007-07-13 9:48 ` Paolo Bonzini @ 2007-07-13 13:42 ` Eric Botcazou 0 siblings, 0 replies; 25+ messages in thread From: Eric Botcazou @ 2007-07-13 13:42 UTC (permalink / raw) To: bonzini; +Cc: gcc-patches > I've reverted it now. Sorry for the useless arguing. No problem. Thanks! -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2007-07-13 13:21 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-07-05 10:54 PR/32004, tree-ssa caused in/out asm constraints to often need reloads Paolo Bonzini [not found] ` <84fc9c000707050401h46157b0fye4b32c3e6b1c8063@mail.gmail.com> 2007-07-05 11:48 ` Paolo Bonzini 2007-07-05 18:34 ` Mark Mitchell 2007-07-05 19:39 ` Paolo Bonzini 2007-07-05 20:24 ` Mark Mitchell 2007-07-05 20:15 ` Paolo Bonzini 2007-07-07 16:29 ` Eric Botcazou 2007-07-07 18:24 ` Andrew Pinski 2007-07-07 18:30 ` Eric Botcazou 2007-07-09 2:23 ` Paolo Bonzini 2007-07-09 6:52 ` Eric Botcazou 2007-07-09 12:09 ` Paolo Bonzini 2007-07-09 12:32 ` Paolo Bonzini 2007-07-09 12:40 ` Richard Guenther 2007-07-09 13:01 ` Eric Botcazou 2007-07-09 13:01 ` Richard Guenther 2007-07-09 13:36 ` Eric Botcazou 2007-07-09 13:12 ` Paolo Bonzini 2007-07-09 13:23 ` Andrew Pinski 2007-07-09 13:45 ` Eric Botcazou 2007-07-09 15:33 ` Paolo Bonzini 2007-07-09 17:25 ` Eric Botcazou 2007-07-09 14:00 ` Eric Botcazou 2007-07-13 9:48 ` Paolo Bonzini 2007-07-13 13:42 ` Eric Botcazou
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).