| Message ID | alpine.LNX.2.00.1011111822410.6883@monoid.intra.ispras.ru |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 11, 2010 at 06:32:28PM +0300, Alexander Monakov wrote: > This is what we need from the selective scheduler side (the part of the > sel-sched.c patch actually needs to be more complicated to work properly, but > the alias.c change is what we have in mind to fix PR45652). That's unfortunately not usable for var-tracking, where we know VEC_index (rtx, reg_base_value, dest_regno) is non-NULL and different from src_regno, yet we want to copy from src_regno instead of clearing it. > +/* Set REG_BASE_VALUE of DEST to that of SRC if unset, > + or set it to NULL if DEST and SRC have conflicting REG_BASE_VALUEs. */ > +void > +inherit_reg_base_value (rtx dest, rtx src) > +{ > + unsigned dest_regno = REGNO (dest), src_regno = REGNO (src); > + > + if (! VEC_index (rtx, reg_base_value, dest_regno)) > + { > + if (!reg_seen[dest_regno] > + && (dest_regno >= FIRST_PSEUDO_REGISTER || !fixed_regs[dest_regno])) > + VEC_replace (rtx, reg_base_value, dest_regno, > + VEC_index (rtx, reg_base_value, src_regno)); > + } > + else if (VEC_index (rtx, reg_base_value, src_regno) > + != VEC_index (rtx, reg_base_value, dest_regno)) > + VEC_replace (rtx, reg_base_value, dest_regno, NULL_RTX); > + > + reg_seen[dest_regno] = 1; > +} > + Jakub
On Thu, Nov 11, 2010 at 4:41 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Nov 11, 2010 at 06:32:28PM +0300, Alexander Monakov wrote: >> This is what we need from the selective scheduler side (the part of the >> sel-sched.c patch actually needs to be more complicated to work properly, but >> the alias.c change is what we have in mind to fix PR45652). > > That's unfortunately not usable for var-tracking, where we know > VEC_index (rtx, reg_base_value, dest_regno) is non-NULL and different from > src_regno, yet we want to copy from src_regno instead of clearing it. Hm, can't we teach alias analysis about cfa_base_reg somehow? What would we loose (in var-tracking) if we re-set the regs base value to NULL instead of to that of the frame pointer? Richard. >> +/* Set REG_BASE_VALUE of DEST to that of SRC if unset, >> + or set it to NULL if DEST and SRC have conflicting REG_BASE_VALUEs. */ >> +void >> +inherit_reg_base_value (rtx dest, rtx src) >> +{ >> + unsigned dest_regno = REGNO (dest), src_regno = REGNO (src); >> + >> + if (! VEC_index (rtx, reg_base_value, dest_regno)) >> + { >> + if (!reg_seen[dest_regno] >> + && (dest_regno >= FIRST_PSEUDO_REGISTER || !fixed_regs[dest_regno])) >> + VEC_replace (rtx, reg_base_value, dest_regno, >> + VEC_index (rtx, reg_base_value, src_regno)); >> + } >> + else if (VEC_index (rtx, reg_base_value, src_regno) >> + != VEC_index (rtx, reg_base_value, dest_regno)) >> + VEC_replace (rtx, reg_base_value, dest_regno, NULL_RTX); >> + >> + reg_seen[dest_regno] = 1; >> +} >> + > > Jakub >
On Fri, Nov 12, 2010 at 10:57:06AM +0100, Richard Guenther wrote: > On Thu, Nov 11, 2010 at 4:41 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Nov 11, 2010 at 06:32:28PM +0300, Alexander Monakov wrote: > >> This is what we need from the selective scheduler side (the part of the > >> sel-sched.c patch actually needs to be more complicated to work properly, but > >> the alias.c change is what we have in mind to fix PR45652). > > > > That's unfortunately not usable for var-tracking, where we know > > VEC_index (rtx, reg_base_value, dest_regno) is non-NULL and different from > > src_regno, yet we want to copy from src_regno instead of clearing it. > > Hm, can't we teach alias analysis about cfa_base_reg somehow? What would Ugh, by repeating all the checks var-tracking does for it? That would be very ugly and very expensive. Remember the tests include e.g. eliminate_regs. If sel-sched invokes this function itself, it would be very slow. > we loose (in var-tracking) if we re-set the regs base value to NULL instead > of to that of the frame pointer? Suddenly cfa_base_rtx based mems would be thought to alias everything, which would make debug info quality significantly worse. I must say I don't understand why the patch sets it to NULL_RTX instead of copying there VEC_index (rtx, reg_base_value, src_regno), by making both have the same value they are handled like they might alias each other, but still not considered to alias unrelated bases. Would sel-sched work with my alias.c change (i.e. just the VEC_replace)? If not, perhaps we should add some additional argument which would select the behavior we want (but that is not very far from just having two different functions). Jakub
On Fri, 12 Nov 2010, Jakub Jelinek wrote: > I must say I don't understand why the patch sets it to NULL_RTX instead of > copying there VEC_index (rtx, reg_base_value, src_regno), by making both > have the same value they are handled like they might alias each other, > but still not considered to alias unrelated bases. If dest_regno already has a reg_base_value, it may have a SET somewhere else in the function. In selective scheduler, we are creating a new SET, and if we just copy reg_base_value from src_regno, we would be changing alias information for the existing SET of the same register. It would be wrong: the existing SET may alias some bases that the new would not. Therefore, we have to drop it to NULL. > Would sel-sched work with my alias.c change (i.e. just the VEC_replace)? No, we reuse registers for renaming, and per the above, we cannot simply replace. Thanks. Alexander
On Fri, Nov 12, 2010 at 12:15 PM, Alexander Monakov <amonakov@ispras.ru> wrote: > > > On Fri, 12 Nov 2010, Jakub Jelinek wrote: > >> I must say I don't understand why the patch sets it to NULL_RTX instead of >> copying there VEC_index (rtx, reg_base_value, src_regno), by making both >> have the same value they are handled like they might alias each other, >> but still not considered to alias unrelated bases. > > If dest_regno already has a reg_base_value, it may have a SET somewhere else > in the function. In selective scheduler, we are creating a new SET, and if we > just copy reg_base_value from src_regno, we would be changing alias > information for the existing SET of the same register. It would be wrong: the > existing SET may alias some bases that the new would not. Therefore, we have > to drop it to NULL. Yep, you'd need to unify the base values, thus, loop over all reg_base_values and change each occurance of either to the other. That would work I think. Don't you need something similar for the cfa_base_reg? Richard.
On Fri, Nov 12, 2010 at 12:54:31PM +0100, Richard Guenther wrote: > Yep, you'd need to unify the base values, thus, loop over all reg_base_values > and change each occurance of either to the other. That would work If the loop is needed, then it would be needed for both the NULL and copy alternative, if the loop is not needed, it is needed for neither variant. REG_BASE_VALUE (dest) = NULL_RTX is changing just that one reg to point to anything, but any other reg which has its old REG_BASE_VALUE too will still contain its old REG_BASE_VALUE. > I think. Don't you need something similar for the cfa_base_reg? I don't, because I know precisely what REG_BASE_VALUE (cfa_base_rtx) is (it is an ADDRESS not used anywhere else). Jakub
diff --git a/gcc/alias.c b/gcc/alias.c index 2e0ac06..0e65d66 100644 --- a/gcc/alias.c +++ b/gcc/alias.c @@ -1291,6 +1291,27 @@ record_set (rtx dest, const_rtx set, void *data ATTRIBUTE_UNUSED) reg_seen[regno] = 1; } +/* Set REG_BASE_VALUE of DEST to that of SRC if unset, + or set it to NULL if DEST and SRC have conflicting REG_BASE_VALUEs. */ +void +inherit_reg_base_value (rtx dest, rtx src) +{ + unsigned dest_regno = REGNO (dest), src_regno = REGNO (src); + + if (! VEC_index (rtx, reg_base_value, dest_regno)) + { + if (!reg_seen[dest_regno] + && (dest_regno >= FIRST_PSEUDO_REGISTER || !fixed_regs[dest_regno])) + VEC_replace (rtx, reg_base_value, dest_regno, + VEC_index (rtx, reg_base_value, src_regno)); + } + else if (VEC_index (rtx, reg_base_value, src_regno) + != VEC_index (rtx, reg_base_value, dest_regno)) + VEC_replace (rtx, reg_base_value, dest_regno, NULL_RTX); + + reg_seen[dest_regno] = 1; +} + /* If a value is known for REGNO, return it. */ rtx @@ -2901,14 +2922,14 @@ init_alias_analysis (void) /* Clean up. */ free (new_reg_base_value); new_reg_base_value = 0; - free (reg_seen); - reg_seen = 0; timevar_pop (TV_ALIAS_ANALYSIS); } void end_alias_analysis (void) { + free (reg_seen); + reg_seen = 0; old_reg_base_value = reg_base_value; ggc_free (reg_known_value); reg_known_value = 0; diff --git a/gcc/rtl.h b/gcc/rtl.h index 3e1df2c..8cc6c5c 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -2508,6 +2508,7 @@ extern rtx find_base_term (rtx); extern rtx gen_hard_reg_clobber (enum machine_mode, unsigned int); extern rtx get_reg_known_value (unsigned int); extern bool get_reg_known_equiv_p (unsigned int); +extern void inherit_reg_base_value (rtx, rtx); #ifdef STACK_REGS extern int stack_regs_mentioned (const_rtx insn); diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c index 70e831d..ab7c708 100644 --- a/gcc/sel-sched.c +++ b/gcc/sel-sched.c @@ -5856,6 +5856,7 @@ maybe_emit_renaming_copy (rtx insn, insn); EXPR_SPEC_DONE_DS (INSN_EXPR (reg_move_insn)) = 0; replace_dest_with_reg_in_expr (params->c_expr, params->dest); + inherit_reg_base_value (params->dest, cur_reg); insn_emitted = true; params->was_renamed = true;