This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: RTL sharing tester (for testing)
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
{