On Sun, Sep 23, 2012 at 07:59:37AM -0300, Alexandre Oliva wrote:
> This patch introduces a global mode of dead_debug tracking for use in
> fast DCE. If a debug use reaches the top of a basic block before
> finding its death point, the pending and subsequent uses of the pseudo
> in debug insns will all be substituted with the same debug temp, and
> death points will get the value bound to the debug temp.
Thanks for working on this. The patch generally looks good, just some minor
nits below.
If it turns out to be too expensive to do global dead debug tracking in
every fast DCE, we could limit it to the first one via some new DF flag.
> * df-problems.c: Adjust.
I think you should list here what functions you've changed.
> --- a/gcc/valtrack.c
> +++ b/gcc/valtrack.c
...
> +static bool
> +dead_debug_global_replace_temp (struct dead_debug_global *global,
> + df_ref use, unsigned int uregno,
> + bitmap *pto_rescan)
> +{
> + if (!global || uregno < FIRST_PSEUDO_REGISTER
> + || !global->used
> + || !bitmap_bit_p (global->used, uregno))
> + return false;
> +
> + gcc_checking_assert (REGNO (*DF_REF_REAL_LOC (use)) == uregno);
> +
> + dead_debug_global_entry *entry
> + = dead_debug_global_find (global, *DF_REF_REAL_LOC (use));
> + gcc_checking_assert (GET_CODE (entry->reg) == REG
> + && REGNO (entry->reg) == uregno);
I think just for safety you should here:
if (GET_MODE (*DF_REF_REAL_LOC (use)) != GET_MODE (entry->reg))
return false;
(the other alternative would be to use mode in the hash function etc.,
but if usually the same pseudo has the same mode everywhere, then the above
should be good enough).
> +static void
> +dead_debug_promote_uses (struct dead_debug_local *debug)
> +{
> + for (struct dead_debug_use *head = debug->head, **headp = &debug->head;
> + head; head = *headp)
> + {
> + rtx reg = *DF_REF_REAL_LOC (head->use);
> +
> + if (GET_CODE (reg) != REG
> + || REGNO (reg) < FIRST_PSEUDO_REGISTER)
> + {
> + headp = &head->next;
> + continue;
> + }
> +
> + if (!debug->global->used)
> + debug->global->used = BITMAP_ALLOC (NULL);
> +
> + if (bitmap_set_bit (debug->global->used, REGNO (reg)))
> + dead_debug_global_insert (debug->global, reg,
> + make_debug_expr_from_rtl (reg));
> +
> + dead_debug_global_replace_temp (debug->global, head->use, REGNO (reg),
> + &debug->to_rescan);
Here you are ignoring the return value from dead_debug_global_replace_temp.
IMHO you should just do what you do a few lines above if it returns false,
something like
if (!dead_debug_global_replace_temp (debug->global, head->use, REGNO (reg),
&debug->to_rescan))
{
headp = &head->next;
continue;
}
> +
> + *headp = head->next;
> + XDELETE (head);
> + }
> +}
> +
> /* Reset all debug insns with pending uses. Release the bitmap in it,
> unless it is USED. USED must be the same bitmap passed to
> dead_debug_init. */
The above comment should be adjusted to dead_debug_local_init.
> - rtx reg = NULL;
> + rtx reg = NULL_RTX;
> rtx breg;
> - rtx dval;
> + rtx dval = NULL_RTX;
> rtx bind;
> + bool global;
>
> - if (!debug->used || !bitmap_clear_bit (debug->used, uregno))
> + if (!debug->used)
> + return 0;
> +
> + global = debug->global && debug->global->used
> + && bitmap_bit_p (debug->global->used, uregno);
I'd think here formatting should put && below debug in "= debug->global".
Perhaps put && debug->global->used onto its own line too?
> +/* Descriptor for hash_table to hash by dead_debug_global_entry's REG
> + and map to DTEMP. */
> +
> +struct dead_debug_hash_descr
> +{
> + /* The hash table contains pointers to entries of this type. */
> + typedef struct dead_debug_global_entry T;
> + /* Hash on the pseudo number. */
> + static inline hashval_t hash (T const *my)
> + {
> + return REGNO (my->reg);
> + }
> + /* Entries are identical if they refer to the same pseudo. */
> + static inline bool equal (T const *my, T const *other)
> + {
> + return my->reg == other->reg;
> + }
> + /* Release entries when they're removed. */
> + static inline void remove (T *p)
> + {
> + XDELETE (p);
> + }
> +};
I believe the coding conventions ask to put the inlines outside of the
class body, see e.g. coverage.c.
> +
> +/* Maintain a global table of pseudos used in debug insns after their
> + deaths in other blocks, and debug temps their deathpoint values are
> + to be bound to. */
> +
> +struct dead_debug_global
> +{
> + /* This hash table that maps pseudos to debug temps. */
> + hash_table<dead_debug_hash_descr> htab;
I think all other hash_table uses put a space here before <.
BTW, the patch fixes most of the (non-LTO) fails left in the
http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00711.html patch, good job!
Jakub
