| Message ID | fc64c2a2-b90e-42d6-856e-11553133b099@EXCHHUB01.MIPS.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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;