PR/32004, tree-ssa caused in/out asm constraints to often need reloads

PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

[-- 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;

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Mark Mitchell]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

>  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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

[-- 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;

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Mark Mitchell]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Andrew Pinski]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Richard Guenther]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Richard Guenther]

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.

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Andrew Pinski]

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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Paolo Bonzini]

> 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

Re: PR/32004, tree-ssa caused in/out asm constraints to often need reloads

[Eric Botcazou]

> I've reverted it now.  Sorry for the useless arguing.

No problem.  Thanks!

-- 
Eric Botcazou