[Patch, PR 54128] ira.c change to fix mips bootstrap

[Patch, PR 54128] ira.c change to fix mips bootstrap

[Steve Ellcey]

Here is my patch to fix the bootstrap comparision failure (PR 54128) on
MIPS.  The reason for the comparision failure was a difference in
register usage and I tracked it down to build_insn_chain which checked
all instructions for register usage in order to set the dead_or_set
and live_relevant_regs bitmaps instead of checking only non-debug
instructions.  Changing INSN_P to NONDEBUG_INSN_P in build_insn_chain
allowed me to bootstrap and caused no regressions.

OK to checkin?

Steve Ellcey
sellcey@mips.com


2012-08-31  Steve Ellcey  <sellcey@mips.com>

	PR bootstrap/54128
	* ira.c (build_insn_chain): Check only NONDEBUG instructions for
	register usage.

diff --git a/gcc/ira.c b/gcc/ira.c
index 3825498..477c87b 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -3341,7 +3341,7 @@ build_insn_chain (void)
 	      c->insn = insn;
 	      c->block = bb->index;
 
-	      if (INSN_P (insn))
+	      if (NONDEBUG_INSN_P (insn))
 		for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
 		  {
 		    df_ref def = *def_rec;
@@ -3432,7 +3432,7 @@ build_insn_chain (void)
 	      bitmap_and_compl_into (live_relevant_regs, elim_regset);
 	      bitmap_copy (&c->live_throughout, live_relevant_regs);
 
-	      if (INSN_P (insn))
+	      if (NONDEBUG_INSN_P (insn))
 		for (use_rec = DF_INSN_UID_USES (uid); *use_rec; use_rec++)
 		  {
 		    df_ref use = *use_rec;

Re: [Patch, PR 54128] ira.c change to fix mips bootstrap

[Jakub Jelinek]

On Fri, Aug 31, 2012 at 10:58:51AM -0700, Steve Ellcey  wrote:
> Here is my patch to fix the bootstrap comparision failure (PR 54128) on
> MIPS.  The reason for the comparision failure was a difference in
> register usage and I tracked it down to build_insn_chain which checked
> all instructions for register usage in order to set the dead_or_set
> and live_relevant_regs bitmaps instead of checking only non-debug
> instructions.  Changing INSN_P to NONDEBUG_INSN_P in build_insn_chain
> allowed me to bootstrap and caused no regressions.

The debug insns generally shouldn't extend the lifetime of pseudos (see the
valtrack.c stuff), so if you hit this, there is probably some earlier bug
that didn't reset/adjust the debug insns in question.
I'm not saying the ira.c patch is absolutely a bad idea, but it would be
good if you could investigate where those debug insns started extending
lifetime of pseudos.

> 2012-08-31  Steve Ellcey  <sellcey@mips.com>
> 
> 	PR bootstrap/54128
> 	* ira.c (build_insn_chain): Check only NONDEBUG instructions for
> 	register usage.

	Jakub

Re: [Patch, PR 54128] ira.c change to fix mips bootstrap

[Steve Ellcey]

On Wed, 2012-09-05 at 08:15 +0200, Jakub Jelinek wrote:

> The debug insns generally shouldn't extend the lifetime of pseudos (see the
> valtrack.c stuff), so if you hit this, there is probably some earlier bug
> that didn't reset/adjust the debug insns in question.
> I'm not saying the ira.c patch is absolutely a bad idea, but it would be
> good if you could investigate where those debug insns started extending
> lifetime of pseudos.

I am not sure I know how to do that.  I am also not sure the problem is
with extending the life of a psuedo register or if it is in recognizing
that a hard register is dead.  $2, the register that doesn't get reused
when generating debug code is the register used to return values.  In
this case I am returning a 64 bit integer value (step_c) that is split
across two registers ($2 and $3).  In the ira dump file I don't see any
debug instructions referring to $3, but I do have one for $2.  The
debug_insn for $2 first shows up in the cse1 phase and there is no
debug_insn for $3, perhaps because we only use the lower half of the
return value.


(debug_insn 73 25 72 5 (var_location:SI D#1 (reg:SI 2 $2)) -1
     (nil))
(insn 72 73 27 5 (set (reg:SI 224 [ step_c+4 ])
        (reg:SI 3 $3 [orig:2+4 ] [2])) x.i:58 282 {*movsi_internal}
     (expr_list:REG_DEAD (reg:SI 3 $3 [orig:2+4 ] [2])
        (nil)))
(debug_insn 27 72 28 5 (var_location:DI step_c (concatn/v:DI [
            (debug_expr:SI D#1)
            (reg:SI 224 [ step_c+4 ])
        ])) x.i:58 -1
     (nil))

It seems odd to have a concatn where one element is a debug_expr and the other
is a register.  But I don't know if this is a problem or a normal way of handling
functions that return a value in two registers.

Steve Ellcey
sje@cup.hp.com

Re: [Patch, PR 54128] ira.c change to fix mips bootstrap

[Steve Ellcey]

On Wed, 2012-09-05 at 08:15 +0200, Jakub Jelinek wrote:
> On Fri, Aug 31, 2012 at 10:58:51AM -0700, Steve Ellcey  wrote:
> > Here is my patch to fix the bootstrap comparision failure (PR 54128) on
> > MIPS.  The reason for the comparision failure was a difference in
> > register usage and I tracked it down to build_insn_chain which checked
> > all instructions for register usage in order to set the dead_or_set
> > and live_relevant_regs bitmaps instead of checking only non-debug
> > instructions.  Changing INSN_P to NONDEBUG_INSN_P in build_insn_chain
> > allowed me to bootstrap and caused no regressions.
> 
> The debug insns generally shouldn't extend the lifetime of pseudos (see the
> valtrack.c stuff), so if you hit this, there is probably some earlier bug
> that didn't reset/adjust the debug insns in question.
> I'm not saying the ira.c patch is absolutely a bad idea, but it would be
> good if you could investigate where those debug insns started extending
> lifetime of pseudos.
> 
> > 2012-08-31  Steve Ellcey  <sellcey@mips.com>
> > 
> > 	PR bootstrap/54128
> > 	* ira.c (build_insn_chain): Check only NONDEBUG instructions for
> > 	register usage.
> 
> 	Jakub

I think I know where this may be going wrong, though I am having trouble
actually creating a patch.  I think MIPS should define
TARGET_DELAY_VARTRACK and call variable_tracking_main from mips_reorg.

The systems that define TARGET_DELAY_VARTRACK all have this comment:

/* Variable tracking should be run after all optimizations which
   change order of insns.  It also needs a valid CFG.  */
#undef TARGET_DELAY_VARTRACK
#define TARGET_DELAY_VARTRACK true

And I think mips_reorg could change the order of insns.  I have tried
putting a call to variable_tracking_main in mips_df_reorg (and changed
mips_cfg_in_reorg to return true if flag_var_tracking is true but that
didn't fix the problem.  I thought this might be because some of the
mips_reorg code that comes after mips_df_reorg is still changing
insn ordering.  I tried putting a call to variable_tracking_main at the
end of mips_reorg:

  if (flag_var_tracking)
    {
      compute_bb_for_insn ();
      df_analyze ();
      timevar_push (TV_VAR_TRACKING);
      variable_tracking_main ();
      timevar_pop (TV_VAR_TRACKING);
      df_finish_pass (false);
      free_bb_for_insn ();
    }

But I just get a seg fault in compute_bb_for_insn, If I remove
that (and free_bb_for_insn) I get a segfault in df_analyze.  I
am not sure exactly what I need to set up to call variable_tracking_main
at this point in the code.  Is there something else I need to call to
ensure that I have a valid control flow graph?  I don't see other
platforms that call variable_tracking_main from their reorg routines
doing anything else.

Steve Ellcey
sellcey@mips.com

Re: [Patch, PR 54128] ira.c change to fix mips bootstrap

[Richard Sandiford]

Don't have time to look at this in detail, but FWIW:

Steve Ellcey <sellcey@mips.com> writes:
> On Wed, 2012-09-05 at 08:15 +0200, Jakub Jelinek wrote:
>> On Fri, Aug 31, 2012 at 10:58:51AM -0700, Steve Ellcey  wrote:
>> > Here is my patch to fix the bootstrap comparision failure (PR 54128) on
>> > MIPS.  The reason for the comparision failure was a difference in
>> > register usage and I tracked it down to build_insn_chain which checked
>> > all instructions for register usage in order to set the dead_or_set
>> > and live_relevant_regs bitmaps instead of checking only non-debug
>> > instructions.  Changing INSN_P to NONDEBUG_INSN_P in build_insn_chain
>> > allowed me to bootstrap and caused no regressions.
>> 
>> The debug insns generally shouldn't extend the lifetime of pseudos (see the
>> valtrack.c stuff), so if you hit this, there is probably some earlier bug
>> that didn't reset/adjust the debug insns in question.
>> I'm not saying the ira.c patch is absolutely a bad idea, but it would be
>> good if you could investigate where those debug insns started extending
>> lifetime of pseudos.
>> 
>> > 2012-08-31  Steve Ellcey  <sellcey@mips.com>
>> > 
>> > 	PR bootstrap/54128
>> > 	* ira.c (build_insn_chain): Check only NONDEBUG instructions for
>> > 	register usage.
>> 
>> 	Jakub
>
> I think I know where this may be going wrong, though I am having trouble
> actually creating a patch.  I think MIPS should define
> TARGET_DELAY_VARTRACK and call variable_tracking_main from mips_reorg.
>
> The systems that define TARGET_DELAY_VARTRACK all have this comment:
>
> /* Variable tracking should be run after all optimizations which
>    change order of insns.  It also needs a valid CFG.  */

That isn't possible on delayed-branch targets.  dbr_schedule changes
the order of insns and (necessarily, until someone rewrites it)
runs with the CFG pulled down.

> And I think mips_reorg could change the order of insns.  I have tried
> putting a call to variable_tracking_main in mips_df_reorg (and changed
> mips_cfg_in_reorg to return true if flag_var_tracking is true but that
> didn't fix the problem.  I thought this might be because some of the
> mips_reorg code that comes after mips_df_reorg is still changing
> insn ordering.

Right.  mips_reorg calls dbr_schedule as a subroutine after mips_df_reorg.
dbr_schedule ought to be the last point at which we change the order of the
instructions though (rather than splitting them, inserting hazard nops, etc.).
So if the comment above really is what vartrack expects, it sounds like
this is still a generic dbr_schedule vs. vartrack thing rather than
a mips_reorg vs. vartrack thing.

Richard

Re: [Patch, PR 54128] ira.c change to fix mips bootstrap

[Jakub Jelinek]

On Fri, Aug 31, 2012 at 10:58:51AM -0700, Steve Ellcey  wrote:
> Here is my patch to fix the bootstrap comparision failure (PR 54128) on
> MIPS.  The reason for the comparision failure was a difference in
> register usage and I tracked it down to build_insn_chain which checked
> all instructions for register usage in order to set the dead_or_set
> and live_relevant_regs bitmaps instead of checking only non-debug
> instructions.  Changing INSN_P to NONDEBUG_INSN_P in build_insn_chain
> allowed me to bootstrap and caused no regressions.
> 
> OK to checkin?

Given Alex' comments in the PR, the second hunk is definitely ok for trunk,
the first one can be applied too (but you can skip it too if you want, it
shouldn't make a difference).

> 2012-08-31  Steve Ellcey  <sellcey@mips.com>
> 
> 	PR bootstrap/54128
> 	* ira.c (build_insn_chain): Check only NONDEBUG instructions for
> 	register usage.
> 
> diff --git a/gcc/ira.c b/gcc/ira.c
> index 3825498..477c87b 100644
> --- a/gcc/ira.c
> +++ b/gcc/ira.c
> @@ -3341,7 +3341,7 @@ build_insn_chain (void)
>  	      c->insn = insn;
>  	      c->block = bb->index;
>  
> -	      if (INSN_P (insn))
> +	      if (NONDEBUG_INSN_P (insn))
>  		for (def_rec = DF_INSN_UID_DEFS (uid); *def_rec; def_rec++)
>  		  {
>  		    df_ref def = *def_rec;
> @@ -3432,7 +3432,7 @@ build_insn_chain (void)
>  	      bitmap_and_compl_into (live_relevant_regs, elim_regset);
>  	      bitmap_copy (&c->live_throughout, live_relevant_regs);
>  
> -	      if (INSN_P (insn))
> +	      if (NONDEBUG_INSN_P (insn))
>  		for (use_rec = DF_INSN_UID_USES (uid); *use_rec; use_rec++)
>  		  {
>  		    df_ref use = *use_rec;

	Jakub

Re: [Patch, PR 54128] ira.c change to fix mips bootstrap

[Steve Ellcey]

On Fri, 2012-12-21 at 08:46 +0100, Jakub Jelinek wrote:

> Given Alex' comments in the PR, the second hunk is definitely ok for trunk,
> the first one can be applied too (but you can skip it too if you want, it
> shouldn't make a difference).


Thanks, I checked in both chunks.

Steve Ellcey
sellcey@mips.com