This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RFA: Stop df from generating SUBREG REG_NOTES
- From: Richard Sandiford <rsandifo at nildram dot co dot uk>
- To: gcc-patches at gcc dot gnu dot org
- Date: Fri, 13 Jul 2007 10:56:45 +0100
- Subject: RFA: Stop df from generating SUBREG REG_NOTES
- References: <20070626000132.GC995@kam.mff.cuni.cz> <468283E0.3010004@google.com> <20070627205004.GB5913@kam.mff.cuni.cz> <20070628.081214.68144053.kkojima@rr.iij4u.or.jp> <20070627232715.GD15535@atrey.karlin.mff.cuni.cz> <87hcosdoqc.fsf@moria.site> <20070628171601.GF5913@kam.mff.cuni.cz> <87myyjkfmr.fsf@firetop.home> <87abujjjnk.fsf@firetop.home>
Ping!
Richard Sandiford <rsandifo@nildram.co.uk> writes:
> Richard Sandiford <rsandifo@nildram.co.uk> writes:
>> Jan Hubicka <jh@suse.cz> writes:
>>> The users of df_set_note all just take the registers from dataflow
>>> information and call the function to produce REG_DEAD/REG_UNUSED notes.
>>> This all is going to introduce invalid sharing for subregs in all cases
>>> and if we are going to produce the note with SUBREG, we simply must
>>> produce the copy. (If we are very very cautious, we probably can have
>>> some cache since we are probably recomputing those notes quite often,
>>> but I doubt it is worth the extra complexity and we can see it easilly
>>> from memory tester)
>>>
>>> Note that copy_rtx does nothing for REG, since registers are shared, so
>>> it should not be overly expensive and we simply must produce the
>>> duplicate for the case of SUBREG or any other unshared RTL expression.
>>>
>>> However I now wonder why do we want to see subregs there at all.
>>> Originally I assumed that we now do partial liveness for multiple word
>>> values, but it is not the case. Perhaps the right thing to do is to
>>> simply strip away the SUBREG and keep only the REG itself?
>>> At least before dataflow, I don't think we produced SUBREGs in those
>>> notes, I might be wrong.
>>
>> FWIW, I agree. Several places in gcc seem to assume that REG_DEAD
>> and REG_UNUSED notes are plain REGs, and access REGNO unconditionally.
>> It even looks like df_ref_record was written with that in mind;
>> there's an "oldreg" variable alongside "reg", presumably so that
>> "reg" could be turned into "SUBREG_REG (oldreg)" if necessary,
>> but the two variables stay the same throughout.
>>
>> This was causing a segmentation fault while building libjava
>> for mipsel-linux-gnu. We had a REG_DEAD (subreg:DI (reg:DF $f0))
>> note, and some code was applying REGNO to that subreg.
>>
>> Also, there seems to be some redundancy in df_mw_hardreg.
>> It has both a mw_reg field and a loc field, but it looks like
>> all uses of loc can be replaced with uses of mw_reg.
>>
>> I'm testing the patch below. I wouldn't normally post a patch
>> before it has finished testing, but since the question is being
>> actively debated, I thought I might as well.
>
> Of course, the problem with doing that is that I changed my mind
> soon afterwards. The patch survived testing, but as discussed
> on #gcc yesterday, I no longer think using the inner REG is
> unconditionally the right thing to do.
>
> The SUBREG handling in df_ref_record assumes that we can ask the port
> for the range of registers that (subreg:M (reg:N R)) occupies, even when
> mode M is not valid for hard register R. This assumption was inherited
> from flow, and I'm assuming that it is correct. (The bug I'm fixing is
> a malformed register note rather than inaccurate liveness information.)
> The range of registers for M is obviously not always going to be the
> same as the range for N, even though it was in the MIPS case.
>
> The code that adds notes will try to use the original mw_reg if
> all of the referenced register is dead (for REG_DEAD) or unused
> (for REG_UNUSED). It will otherwise create notes for each individual
> hard register.
>
> I think we should always take the latter approach -- creating individual
> notes for each hard register -- when the mw_reg is a SUBREG rather than
> a REG. This ties in quite naturally with the DF_REF_PARTIAL setting;
> as far as I can tell, we always consider a reference to a SUBREG of a
> multiword hard register to be partial, even if the SUBREG appears to be
> the same width as the inner register. (And if we decide to drop the
> DF_REF_PARTIAL in some cases, the approach taken in the patch below
> should still work for the cases that remain DF_REF_PARTIAL.)
>
> Kenny mentioned the old flow code on #gcc yesterday. I think flow
> used to treat a _use_ of a SUBREG like a use of the whole SUBREG_REG.
>> From mark_used_regs:
>
> case SUBREG:
> #ifdef CANNOT_CHANGE_MODE_CLASS
> if (flags & PROP_REG_INFO)
> record_subregs_of_mode (x);
> #endif
>
> /* While we're here, optimize this case. */
> x = SUBREG_REG (x);
> if (!REG_P (x))
> goto retry;
> /* Fall through. */
>
> case REG:
> /* See a register other than being set => mark it as needed. */
> mark_used_reg (pbi, x, cond, insn);
> return;
>
> df is now trying to be more accurate by tracking individual hard
> registers in the SUBREG_REG. The notes we add must clearly agree
> with the main df liveness information, so I don't think the old
> flow code sets any precedent here. We're now tracking more
> registers individually, so adding more individual notes seems
> fair game.
>
> Flow used to track sets individually. From mark_sets_1:
>
> case SUBREG:
> if (REG_P (SUBREG_REG (reg)))
> {
> enum machine_mode outer_mode = GET_MODE (reg);
> enum machine_mode inner_mode = GET_MODE (SUBREG_REG (reg));
>
> /* Identify the range of registers affected. This is moderately
> tricky for hard registers. See alter_subreg. */
>
> regno_last = regno_first = REGNO (SUBREG_REG (reg));
> if (regno_first < FIRST_PSEUDO_REGISTER)
> {
> regno_first += subreg_regno_offset (regno_first, inner_mode,
> SUBREG_BYTE (reg),
> outer_mode);
> regno_last = (regno_first
> + hard_regno_nregs[regno_first][outer_mode] - 1);
>
> /* Since we've just adjusted the register number ranges, make
> sure REG matches. Otherwise some_was_live will be clear
> when it shouldn't have been, and we'll create incorrect
> REG_UNUSED notes. */
> reg = gen_rtx_REG (outer_mode, regno_first);
> }
>
> If every register in [regno_first, regno_last] was unused, flow would
> add a REG_UNUSED note for the REG created above. If only some of those
> registers were unused, flow would add notes for the individual unused
> registers. As I said on IRC yesterday, I'm not convinced the first
> behaviour was a good idea. That REG might well be invalid (otherwise
> why have a SUBREG in the first place?) and it's better not to have
> invalid registers floating around.
>
> Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested by
> building mipsel-linux-gnu (which failed before the patch). OK to install?
>
> Richard
>
>
> gcc/
> * df.h (df_mw_hardreg): Remove "loc" field.
> * df-scan.c (df_ref_record): Don't set it. Remove redundant
> local variable.
> * df-problems.c (df_whole_mw_reg_unused_p): New function,
> split out from df_set_unused_notes_for_mw. Return false for
> partial references. Assert that mw_reg is a REG when returning true.
> (df_set_unused_notes_for_mw): Use it. Use mw_reg instead of *loc.
> (df_whole_mw_reg_dead_p): New function, split out from
> df_set_dead_notes_for_mw. Return false for partial references.
> Assert that mw_reg is a REG when returning true.
> (df_set_dead_notes_for_mw): Use it. Use mw_reg instead of *loc.
> Remove redundant bitmap check.
>
> Index: gcc/df.h
> ===================================================================
> --- gcc/df.h (revision 126108)
> +++ gcc/df.h (working copy)
> @@ -312,7 +312,6 @@ struct dataflow
> struct df_mw_hardreg
> {
> rtx mw_reg; /* The multiword hardreg. */
> - rtx *loc; /* The location of the reg. */
> enum df_ref_type type; /* Used to see if the ref is read or write. */
> enum df_ref_flags flags; /* Various flags. */
> unsigned int start_regno; /* First word of the multi word subreg. */
> Index: gcc/df-scan.c
> ===================================================================
> --- gcc/df-scan.c (revision 126108)
> +++ gcc/df-scan.c (working copy)
> @@ -2627,7 +2627,6 @@ df_ref_record (struct df_collection_rec
> enum df_ref_type ref_type,
> enum df_ref_flags ref_flags)
> {
> - rtx oldreg = reg;
> unsigned int regno;
>
> gcc_assert (REG_P (reg) || GET_CODE (reg) == SUBREG);
> @@ -2658,7 +2657,7 @@ df_ref_record (struct df_collection_rec
> {
> /* Sets to a subreg of a multiword register are partial.
> Sets to a non-subreg of a multiword register are not. */
> - if (GET_CODE (oldreg) == SUBREG)
> + if (GET_CODE (reg) == SUBREG)
> ref_flags |= DF_REF_PARTIAL;
> ref_flags |= DF_REF_MW_HARDREG;
>
> @@ -2666,7 +2665,6 @@ df_ref_record (struct df_collection_rec
> hardreg->type = ref_type;
> hardreg->flags = ref_flags;
> hardreg->mw_reg = reg;
> - hardreg->loc = loc;
> hardreg->start_regno = regno;
> hardreg->end_regno = endregno - 1;
> hardreg->mw_order = df->ref_order++;
> Index: gcc/df-problems.c
> ===================================================================
> --- gcc/df-problems.c (revision 126109)
> +++ gcc/df-problems.c (working copy)
> @@ -3717,6 +3717,32 @@ df_set_note (enum reg_note note_type, rt
> return old;
> }
>
> +/* A subroutine of df_set_unused_notes_for_mw, with a selection of its
> + arguments. Return true if the register value described by MWS's
> + mw_reg is known to be completely unused, and if mw_reg can therefore
> + be used in a REG_UNUSED note. */
> +
> +static bool
> +df_whole_mw_reg_unused_p (struct df_mw_hardreg *mws,
> + bitmap live, bitmap artificial_uses)
> +{
> + unsigned int r;
> +
> + /* If MWS describes a partial reference, create REG_UNUSED notes for
> + individual hard registers. */
> + if (mws->flags & DF_REF_PARTIAL)
> + return false;
> +
> + /* Likewise if some part of the register is used. */
> + for (r = mws->start_regno; r <= mws->end_regno; r++)
> + if (bitmap_bit_p (live, r)
> + || bitmap_bit_p (artificial_uses, r))
> + return false;
> +
> + gcc_assert (REG_P (mws->mw_reg));
> + return true;
> +}
> +
> /* Set the REG_UNUSED notes for the multiword hardreg defs in INSN
> based on the bits in LIVE. Do not generate notes for registers in
> artificial uses. DO_NOT_GEN is updated so that REG_DEAD notes are
> @@ -3729,7 +3755,6 @@ df_set_unused_notes_for_mw (rtx insn, rt
> bitmap live, bitmap do_not_gen,
> bitmap artificial_uses)
> {
> - bool all_dead = true;
> unsigned int r;
>
> #ifdef REG_DEAD_DEBUGGING
> @@ -3737,18 +3762,11 @@ df_set_unused_notes_for_mw (rtx insn, rt
> fprintf (dump_file, "mw_set_unused looking at mws[%d..%d]\n",
> mws->start_regno, mws->end_regno);
> #endif
> - for (r=mws->start_regno; r <= mws->end_regno; r++)
> - if ((bitmap_bit_p (live, r))
> - || bitmap_bit_p (artificial_uses, r))
> - {
> - all_dead = false;
> - break;
> - }
> -
> - if (all_dead)
> +
> + if (df_whole_mw_reg_unused_p (mws, live, artificial_uses))
> {
> unsigned int regno = mws->start_regno;
> - old = df_set_note (REG_UNUSED, insn, old, *(mws->loc));
> + old = df_set_note (REG_UNUSED, insn, old, mws->mw_reg);
>
> #ifdef REG_DEAD_DEBUGGING
> df_print_note ("adding 1: ", insn, REG_NOTES (insn));
> @@ -3773,6 +3791,34 @@ df_set_unused_notes_for_mw (rtx insn, rt
> }
>
>
> +/* A subroutine of df_set_dead_notes_for_mw, with a selection of its
> + arguments. Return true if the register value described by MWS's
> + mw_reg is known to be completely dead, and if mw_reg can therefore
> + be used in a REG_DEAD note. */
> +
> +static bool
> +df_whole_mw_reg_dead_p (struct df_mw_hardreg *mws,
> + bitmap live, bitmap artificial_uses,
> + bitmap do_not_gen)
> +{
> + unsigned int r;
> +
> + /* If MWS describes a partial reference, create REG_DEAD notes for
> + individual hard registers. */
> + if (mws->flags & DF_REF_PARTIAL)
> + return false;
> +
> + /* Likewise if some part of the register is not dead. */
> + for (r = mws->start_regno; r <= mws->end_regno; r++)
> + if (bitmap_bit_p (live, r)
> + || bitmap_bit_p (artificial_uses, r)
> + || bitmap_bit_p (do_not_gen, r))
> + return false;
> +
> + gcc_assert (REG_P (mws->mw_reg));
> + return true;
> +}
> +
> /* Set the REG_DEAD notes for the multiword hardreg use in INSN based
> on the bits in LIVE. DO_NOT_GEN is used to keep REG_DEAD notes
> from being set if the instruction both reads and writes the
> @@ -3783,7 +3829,6 @@ df_set_dead_notes_for_mw (rtx insn, rtx
> bitmap live, bitmap do_not_gen,
> bitmap artificial_uses)
> {
> - bool all_dead = true;
> unsigned int r;
>
> #ifdef REG_DEAD_DEBUGGING
> @@ -3799,25 +3844,13 @@ df_set_dead_notes_for_mw (rtx insn, rtx
> }
> #endif
>
> - for (r = mws->start_regno; r <= mws->end_regno; r++)
> - if ((bitmap_bit_p (live, r))
> - || bitmap_bit_p (artificial_uses, r)
> - || bitmap_bit_p (do_not_gen, r))
> - {
> - all_dead = false;
> - break;
> - }
> -
> - if (all_dead)
> + if (df_whole_mw_reg_dead_p (mws, live, artificial_uses, do_not_gen))
> {
> - if (!bitmap_bit_p (do_not_gen, mws->start_regno))
> - {
> - /* Add a dead note for the entire multi word register. */
> - old = df_set_note (REG_DEAD, insn, old, *(mws->loc));
> + /* Add a dead note for the entire multi word register. */
> + old = df_set_note (REG_DEAD, insn, old, mws->mw_reg);
> #ifdef REG_DEAD_DEBUGGING
> - df_print_note ("adding 1: ", insn, REG_NOTES (insn));
> + df_print_note ("adding 1: ", insn, REG_NOTES (insn));
> #endif
> - }
> }
> else
> {