| Message ID | 20110704052106.GA5131@intel.com |
|---|---|
| State | New |
| Headers | show |
"H.J. Lu" <hongjiu.lu@intel.com> writes:
> RTL-based forward propagation pass shouldn't propagate hard register.
That's seems a bit draconian. Many fixed hard registers ought to be OK.
E.g. there doesn't seem to be anything wrong with propagating uses of
the stack or frame pointers, subject to the usual availability checks.
To play devil's advocate, an alternative might be to
(a) make local_ref_killed_between_p return true for non-fixed hard
registers when a call or asm comes between the two instructions
(b) make use_killed_between return true for non-fixed hard registers
when the instructions are in different basic blocks
Thoughts?
Richard
On Mon, Jul 4, 2011 at 12:57 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > "H.J. Lu" <hongjiu.lu@intel.com> writes: >> RTL-based forward propagation pass shouldn't propagate hard register. > > That's seems a bit draconian. Many fixed hard registers ought to be OK. > E.g. there doesn't seem to be anything wrong with propagating uses of > the stack or frame pointers, subject to the usual availability checks. > > To play devil's advocate, an alternative might be to > > (a) make local_ref_killed_between_p return true for non-fixed hard > registers when a call or asm comes between the two instructions > > (b) make use_killed_between return true for non-fixed hard registers > when the instructions are in different basic blocks > > Thoughts? > There are a few problems with this suggestions: 1. The comments says: /* If USE is a subreg, see if it can be replaced by a pseudo. */ static bool forward_propagate_subreg (df_ref use, rtx def_insn, rtx def_set) { It indicates this function is intended to work on pseudo registers. 2. propagate_rtx avoids hard registers: static rtx propagate_rtx (rtx x, enum machine_mode mode, rtx old_rtx, rtx new_rtx, bool speed) { rtx tem; bool collapsed; int flags; if (REG_P (new_rtx) && REGNO (new_rtx) < FIRST_PSEUDO_REGISTER) return NULL_RTX; It seems that fwprop is intended to deal with pseudo registers. If we want to extend it to hard registers, that should be a separate project. Thanks.
On Mon, Jul 4, 2011 at 1:52 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Jul 4, 2011 at 12:57 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> "H.J. Lu" <hongjiu.lu@intel.com> writes: >>> RTL-based forward propagation pass shouldn't propagate hard register. >> >> That's seems a bit draconian. Many fixed hard registers ought to be OK. >> E.g. there doesn't seem to be anything wrong with propagating uses of >> the stack or frame pointers, subject to the usual availability checks. >> >> To play devil's advocate, an alternative might be to >> >> (a) make local_ref_killed_between_p return true for non-fixed hard >> registers when a call or asm comes between the two instructions >> >> (b) make use_killed_between return true for non-fixed hard registers >> when the instructions are in different basic blocks >> >> Thoughts? >> > > There are a few problems with this suggestions: > > 1. The comments says: > > /* If USE is a subreg, see if it can be replaced by a pseudo. */ > > static bool > forward_propagate_subreg (df_ref use, rtx def_insn, rtx def_set) > { > > It indicates this function is intended to work on pseudo registers. > > 2. propagate_rtx avoids hard registers: > > static rtx > propagate_rtx (rtx x, enum machine_mode mode, rtx old_rtx, rtx new_rtx, > bool speed) > { > rtx tem; > bool collapsed; > int flags; > > if (REG_P (new_rtx) && REGNO (new_rtx) < FIRST_PSEUDO_REGISTER) > return NULL_RTX; > > It seems that fwprop is intended to deal with pseudo registers. If we > want to extend it to hard registers, that should be a separate project. > > Thanks. forward_propagate_subreg issue was introduced by http://gcc.gnu.org/ml/gcc-patches/2009-08/msg01203.html Before that, fwprop never tries to work on hard registers. Alan, is your change to process hard registers intentional? Thanks.
On Mon, Jul 04, 2011 at 01:57:34PM -0700, H.J. Lu wrote: > forward_propagate_subreg issue was introduced by > > http://gcc.gnu.org/ml/gcc-patches/2009-08/msg01203.html > > Before that, fwprop never tries to work on hard registers. I question this claim. It seems to me that fwprop did look at paradoxical subregs of hard regs before my change. > Alan, > is your change to process hard registers intentional? I didn't set out to do anything special with hard regs one way or the other, just extended what was already done for paradoxical subregs to sign and zero extended subregs.
On Mon, Jul 4, 2011 at 4:54 PM, Alan Modra <amodra@gmail.com> wrote: > On Mon, Jul 04, 2011 at 01:57:34PM -0700, H.J. Lu wrote: >> forward_propagate_subreg issue was introduced by >> >> http://gcc.gnu.org/ml/gcc-patches/2009-08/msg01203.html >> >> Before that, fwprop never tries to work on hard registers. > > I question this claim. It seems to me that fwprop did look at > paradoxical subregs of hard regs before my change. I should have said " fwprop never tries to work on zero/sign-extended hard registers." >> Alan, >> is your change to process hard registers intentional? > > I didn't set out to do anything special with hard regs one way or the > other, just extended what was already done for paradoxical subregs to > sign and zero extended subregs. > Does your change depend on processing zero/sign-extended hard registers?
On Mon, Jul 04, 2011 at 05:09:28PM -0700, H.J. Lu wrote: > On Mon, Jul 4, 2011 at 4:54 PM, Alan Modra <amodra@gmail.com> wrote: > > I didn't set out to do anything special with hard regs one way or the > > other, just extended what was already done for paradoxical subregs to > > sign and zero extended subregs. > > Does your change depend on processing zero/sign-extended > hard registers? At the time I wrote the patch I was more interested in pseudos. I expect that powerpc64 won't be greatly affected if hard regs were excluded from this fwprop optimization, but you need to discuss your patch with maintainers of this code. My opinion as a one-time contributor to fwprop doesn't count for much.
On 07/05/2011 01:54 AM, Alan Modra wrote: > > Before that, fwprop never tries to work on hard registers. > > I question this claim. It seems to me that fwprop did look at > paradoxical subregs of hard regs before my change. That wasn't part of the design anyway. The main purpose of fwprop's paradoxical subreg propagation is to get rid of back-to-back SI-to-QI-to-SI conversions, and working with pseudos is enough. The patch is okay as far as I'm concerned, but I'm not a maintainer of fwprop. Perhaps it could be conditionalized on SMALL_REGISTER_CLASSES (that's the source of the bug, I think: the single-register class "D" conflicts with an argument register) but I don't think it's worth it. Paolo
Paolo Bonzini <bonzini@gnu.org> writes: > On 07/05/2011 01:54 AM, Alan Modra wrote: >> > Before that, fwprop never tries to work on hard registers. >> >> I question this claim. It seems to me that fwprop did look at >> paradoxical subregs of hard regs before my change. > > That wasn't part of the design anyway. The main purpose of fwprop's > paradoxical subreg propagation is to get rid of back-to-back > SI-to-QI-to-SI conversions, and working with pseudos is enough. OK, in that case H.J., please go ahead. > The patch is okay as far as I'm concerned, but I'm not a maintainer of > fwprop. You probably should be :-) Richard
On 07/05/2011 10:51 AM, Richard Sandiford wrote: > > The patch is okay as far as I'm concerned, but I'm not a maintainer of > > fwprop. > > You probably should be:-) I'd have no problem with that! Paolo
diff --git a/gcc/fwprop.c b/gcc/fwprop.c index b2fd955..c8009d0 100644 --- a/gcc/fwprop.c +++ b/gcc/fwprop.c @@ -1101,6 +1101,7 @@ forward_propagate_subreg (df_ref use, rtx def_insn, rtx def_set) src = SET_SRC (def_set); if (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src)) + && REGNO (SUBREG_REG (src)) >= FIRST_PSEUDO_REGISTER && GET_MODE (SUBREG_REG (src)) == use_mode && subreg_lowpart_p (src) && all_uses_available_at (def_insn, use_insn)) @@ -1119,6 +1120,7 @@ forward_propagate_subreg (df_ref use, rtx def_insn, rtx def_set) if ((GET_CODE (src) == ZERO_EXTEND || GET_CODE (src) == SIGN_EXTEND) && REG_P (XEXP (src, 0)) + && REGNO (XEXP (src, 0)) >= FIRST_PSEUDO_REGISTER && GET_MODE (XEXP (src, 0)) == use_mode && !free_load_extend (src, def_insn) && all_uses_available_at (def_insn, use_insn))