On Wed, 10 May 2023, [email protected] wrote:
> From: Pan Li <[email protected]>
>
> The decl_or_value is defined as void * before this PATCH. It will take
> care of both the tree_node and rtx_def. Unfortunately, given a void
> pointer cannot tell the input is tree_node or rtx_def.
>
> Then we have some implicit structure layout requirement similar as
> below. Or we will touch unreasonable bits when cast void * to tree_node
> or rtx_def.
>
> +--------+-----------+----------+
> | offset | tree_node | rtx_def |
> +--------+-----------+----------+
> | 0 | code: 16 | code: 16 | <- require the location and bitssize
> +--------+-----------+----------+
> | 16 | ... | mode: 8 |
> +--------+-----------+----------+
> | ... |
> +--------+-----------+----------+
> | 24 | ... | ... |
> +--------+-----------+----------+
>
> This behavior blocks the PATCH that extend the rtx_def mode from 8 to
> 16 bits for running out of machine mode. This PATCH introduced the
> pointer_mux to tell the input is tree_node or rtx_def, and decouple
> the above implicition dependency.
>
> Signed-off-by: Pan Li <[email protected]>
> Co-Authored-By: Richard Sandiford <[email protected]>
> Co-Authored-By: Richard Biener <[email protected]>
>
> gcc/ChangeLog:
>
> * var-tracking.cc (DECL_OR_VALUE_OR_DEFAULT): New macro for
> clean code.
> (dv_is_decl_p): Adjust type changes to pointer_mux.
> (dv_as_decl): Likewise.
> (dv_as_value): Likewise.
> (dv_as_opaque): Likewise.
> (variable_hasher::equal): Likewise.
> (dv_from_decl): Likewise.
> (dv_from_value): Likewise.
> (shared_hash_find_slot_unshare_1): Likewise.
> (shared_hash_find_slot_1): Likewise.
> (shared_hash_find_slot_noinsert_1): Likewise.
> (shared_hash_find_1): Likewise.
> (unshare_variable): Likewise.
> (vars_copy): Likewise.
> (find_loc_in_1pdv): Likewise.
> (find_mem_expr_in_1pdv): Likewise.
> (dataflow_set_different): Likewise.
> (variable_from_dropped): Likewise.
> (variable_was_changed): Likewise.
> (loc_exp_insert_dep): Likewise.
> (notify_dependents_of_resolved_value): Likewise.
> (vt_expand_loc_callback): Likewise.
> (remove_value_from_changed_variables): Likewise.
> (notify_dependents_of_changed_value): Likewise.
> (emit_notes_for_differences_1): Likewise.
> (emit_notes_for_differences_2): Likewise.
> ---
> gcc/var-tracking.cc | 119 +++++++++++++++++++++++++++-----------------
> 1 file changed, 74 insertions(+), 45 deletions(-)
>
> diff --git a/gcc/var-tracking.cc b/gcc/var-tracking.cc
> index fae0c73e02f..9bc9d21e5ba 100644
> --- a/gcc/var-tracking.cc
> +++ b/gcc/var-tracking.cc
> @@ -116,9 +116,17 @@
> #include "fibonacci_heap.h"
> #include "print-rtl.h"
> #include "function-abi.h"
> +#include "mux-utils.h"
>
> typedef fibonacci_heap <long, basic_block_def> bb_heap_t;
>
> +/* A declaration of a variable, or an RTL value being handled like a
> + declaration by pointer_mux. */
> +typedef pointer_mux<tree_node, rtx_def> decl_or_value;
> +
> +#define DECL_OR_VALUE_OR_DEFAULT(ptr) \
> + ((ptr) ? decl_or_value (ptr) : decl_or_value ())
> +
> /* var-tracking.cc assumes that tree code with the same value as VALUE rtx
> code
> has no chance to appear in REG_EXPR/MEM_EXPRs and isn't a decl.
> Currently the value is the same as IDENTIFIER_NODE, which has such
> @@ -196,15 +204,21 @@ struct micro_operation
> };
>
>
> -/* A declaration of a variable, or an RTL value being handled like a
> - declaration. */
> -typedef void *decl_or_value;
> -
> /* Return true if a decl_or_value DV is a DECL or NULL. */
> static inline bool
> dv_is_decl_p (decl_or_value dv)
> {
> - return !dv || (int) TREE_CODE ((tree) dv) != (int) VALUE;
> + bool is_decl = !dv;
> +
> + if (dv)
> + {
> + if (dv.is_first ())
> + is_decl = (int) TREE_CODE (dv.known_first ()) != (int) VALUE;
> + else if (!dv.is_first () && !dv.is_second ())
> + is_decl = true;
> + }
> +
> + return is_decl;
This all looks very confused, shouldn't it just be
return dv.is_first ();
? All the keying on VALUE should no longer be necessary.
> }
>
> /* Return true if a decl_or_value is a VALUE rtl. */
> @@ -219,7 +233,7 @@ static inline tree
> dv_as_decl (decl_or_value dv)
> {
> gcc_checking_assert (dv_is_decl_p (dv));
> - return (tree) dv;
> + return dv.is_first () ? dv.known_first () : NULL_TREE;
and this should be
return dv.known_first ();
? (knowing that ptr-mux will not mutate 'first' and thus preserves
a nullptr there)
> }
>
> /* Return the value in the decl_or_value. */
> @@ -227,14 +241,20 @@ static inline rtx
> dv_as_value (decl_or_value dv)
> {
> gcc_checking_assert (dv_is_value_p (dv));
> - return (rtx)dv;
> + return dv.is_second () ? dv.known_second () : NULL_RTX;;
return dv.known_second (); (the assert makes sure it isn't nullptr)
> }
>
> /* Return the opaque pointer in the decl_or_value. */
> static inline void *
> dv_as_opaque (decl_or_value dv)
> {
> - return dv;
> + if (dv.is_first ())
> + return dv.known_first ();
> +
> + if (dv.is_second ())
> + return dv.known_second ();
> +
> + return NULL;
> }
I think this function should go away - for equality compares
ptr-mux should probably get an operator== overload (or alternatively
an access to the raw pointer value). For cases like
gcc_checking_assert (loc != dv_as_opaque (var->dv));
one could also use var->dv.second_or_null (). But as said
most uses are doing sth like
if (dv_as_opaque (list->dv) == dv_as_opaque (cdv)
which should just become
if (list->dv == cdv)
Richard - any preference here? Go for operator==/!= or go
for an accessor to m_ptr (maybe as uintptr_t)?
>
> @@ -503,9 +523,7 @@ variable_hasher::hash (const variable *v)
> inline bool
> variable_hasher::equal (const variable *v, const void *y)
> {
> - decl_or_value dv = CONST_CAST2 (decl_or_value, const void *, y);
> -
> - return (dv_as_opaque (v->dv) == dv_as_opaque (dv));
> + return dv_as_opaque (v->dv) == y;
> }
>
> /* Free the element of VARIABLE_HTAB (its type is struct variable_def). */
> @@ -1396,8 +1414,7 @@ onepart_pool_allocate (onepart_enum onepart)
> static inline decl_or_value
> dv_from_decl (tree decl)
> {
> - decl_or_value dv;
> - dv = decl;
> + decl_or_value dv = DECL_OR_VALUE_OR_DEFAULT (decl);
it should work to do
devl_or_value dv (decl);
no? Alternatively
devl_or_value dv = decl_or_value::first (decl);
> gcc_checking_assert (dv_is_decl_p (dv));
> return dv;
> }
> @@ -1406,8 +1423,7 @@ dv_from_decl (tree decl)
> static inline decl_or_value
> dv_from_value (rtx value)
> {
> - decl_or_value dv;
> - dv = value;
> + decl_or_value dv = DECL_OR_VALUE_OR_DEFAULT (value);
> gcc_checking_assert (dv_is_value_p (dv));
the later assert here asserts 'value' wasn't nullptr
> return dv;
> }
> @@ -1661,7 +1677,8 @@ shared_hash_find_slot_unshare_1 (shared_hash **pvars,
> decl_or_value dv,
> {
> if (shared_hash_shared (*pvars))
> *pvars = shared_hash_unshare (*pvars);
> - return shared_hash_htab (*pvars)->find_slot_with_hash (dv, dvhash, ins);
> + return shared_hash_htab (*pvars)->find_slot_with_hash (dv_as_opaque (dv),
> + dvhash, ins);
OK, it's probably safes to keep dv_as_opaque for these uses.
Thanks a lot for doing this cleanup.
Richard.
> }
>
> static inline variable **
> @@ -1678,7 +1695,8 @@ shared_hash_find_slot_unshare (shared_hash **pvars,
> decl_or_value dv,
> static inline variable **
> shared_hash_find_slot_1 (shared_hash *vars, decl_or_value dv, hashval_t
> dvhash)
> {
> - return shared_hash_htab (vars)->find_slot_with_hash (dv, dvhash,
> + return shared_hash_htab (vars)->find_slot_with_hash (dv_as_opaque (dv),
> + dvhash,
> shared_hash_shared (vars)
> ? NO_INSERT : INSERT);
> }
> @@ -1695,7 +1713,8 @@ static inline variable **
> shared_hash_find_slot_noinsert_1 (shared_hash *vars, decl_or_value dv,
> hashval_t dvhash)
> {
> - return shared_hash_htab (vars)->find_slot_with_hash (dv, dvhash,
> NO_INSERT);
> + return shared_hash_htab (vars)->find_slot_with_hash (dv_as_opaque (dv),
> + dvhash, NO_INSERT);
> }
>
> static inline variable **
> @@ -1710,7 +1729,7 @@ shared_hash_find_slot_noinsert (shared_hash *vars,
> decl_or_value dv)
> static inline variable *
> shared_hash_find_1 (shared_hash *vars, decl_or_value dv, hashval_t dvhash)
> {
> - return shared_hash_htab (vars)->find_with_hash (dv, dvhash);
> + return shared_hash_htab (vars)->find_with_hash (dv_as_opaque (dv), dvhash);
> }
>
> static inline variable *
> @@ -1807,7 +1826,7 @@ unshare_variable (dataflow_set *set, variable **slot,
> variable *var,
> if (var->in_changed_variables)
> {
> variable **cslot
> - = changed_variables->find_slot_with_hash (var->dv,
> + = changed_variables->find_slot_with_hash (dv_as_opaque (var->dv),
> dv_htab_hash (var->dv),
> NO_INSERT);
> gcc_assert (*cslot == (void *) var);
> @@ -1831,8 +1850,8 @@ vars_copy (variable_table_type *dst,
> variable_table_type *src)
> {
> variable **dstp;
> var->refcount++;
> - dstp = dst->find_slot_with_hash (var->dv, dv_htab_hash (var->dv),
> - INSERT);
> + dstp = dst->find_slot_with_hash (dv_as_opaque (var->dv),
> + dv_htab_hash (var->dv), INSERT);
> *dstp = var;
> }
> }
> @@ -3286,7 +3305,7 @@ find_loc_in_1pdv (rtx loc, variable *var,
> variable_table_type *vars)
> gcc_checking_assert (!node->next);
>
> dv = dv_from_value (node->loc);
> - rvar = vars->find_with_hash (dv, dv_htab_hash (dv));
> + rvar = vars->find_with_hash (dv_as_opaque (dv), dv_htab_hash (dv));
> return find_loc_in_1pdv (loc, rvar, vars);
> }
>
> @@ -4664,7 +4683,7 @@ find_mem_expr_in_1pdv (tree expr, rtx val,
> variable_table_type *vars)
> && !VALUE_RECURSED_INTO (val));
>
> dv = dv_from_value (val);
> - var = vars->find_with_hash (dv, dv_htab_hash (dv));
> + var = vars->find_with_hash (dv_as_opaque (dv), dv_htab_hash (dv));
>
> if (!var)
> return NULL;
> @@ -5103,7 +5122,8 @@ dataflow_set_different (dataflow_set *old_set,
> dataflow_set *new_set)
> var1, variable, hi)
> {
> variable_table_type *htab = shared_hash_htab (new_set->vars);
> - variable *var2 = htab->find_with_hash (var1->dv, dv_htab_hash
> (var1->dv));
> + variable *var2 = htab->find_with_hash (dv_as_opaque (var1->dv),
> + dv_htab_hash (var1->dv));
>
> if (!var2)
> {
> @@ -5140,7 +5160,8 @@ dataflow_set_different (dataflow_set *old_set,
> dataflow_set *new_set)
> var1, variable, hi)
> {
> variable_table_type *htab = shared_hash_htab (old_set->vars);
> - variable *var2 = htab->find_with_hash (var1->dv, dv_htab_hash
> (var1->dv));
> + variable *var2 = htab->find_with_hash (dv_as_opaque (var1->dv),
> + dv_htab_hash (var1->dv));
> if (!var2)
> {
> if (details)
> @@ -7422,7 +7443,8 @@ variable_from_dropped (decl_or_value dv, enum
> insert_option insert)
> variable *empty_var;
> onepart_enum onepart;
>
> - slot = dropped_values->find_slot_with_hash (dv, dv_htab_hash (dv), insert);
> + slot = dropped_values->find_slot_with_hash (dv_as_opaque (dv),
> + dv_htab_hash (dv), insert);
>
> if (!slot)
> return NULL;
> @@ -7493,7 +7515,8 @@ variable_was_changed (variable *var, dataflow_set *set)
> /* Remember this decl or VALUE has been added to changed_variables. */
> set_dv_changed (var->dv, true);
>
> - slot = changed_variables->find_slot_with_hash (var->dv, hash, INSERT);
> + slot = changed_variables->find_slot_with_hash (dv_as_opaque (var->dv),
> + hash, INSERT);
>
> if (*slot)
> {
> @@ -7520,9 +7543,8 @@ variable_was_changed (variable *var, dataflow_set *set)
>
> if (onepart == ONEPART_VALUE || onepart == ONEPART_DEXPR)
> {
> - dslot = dropped_values->find_slot_with_hash (var->dv,
> - dv_htab_hash
> (var->dv),
> - INSERT);
> + dslot = dropped_values->find_slot_with_hash (
> + dv_as_opaque (var->dv), dv_htab_hash (var->dv), INSERT);
> empty_var = *dslot;
>
> if (empty_var)
> @@ -8199,7 +8221,7 @@ loc_exp_insert_dep (variable *var, rtx x,
> variable_table_type *vars)
>
> /* ??? Build a vector of variables parallel to EXPANDING, to avoid
> an additional look up? */
> - xvar = vars->find_with_hash (dv, dv_htab_hash (dv));
> + xvar = vars->find_with_hash (dv_as_opaque (dv), dv_htab_hash (dv));
>
> if (!xvar)
> {
> @@ -8211,7 +8233,8 @@ loc_exp_insert_dep (variable *var, rtx x,
> variable_table_type *vars)
> arise if say the same value appears in two complex expressions in
> the same loc_list, or even more than once in a single
> expression. */
> - if (VAR_LOC_DEP_LST (xvar) && VAR_LOC_DEP_LST (xvar)->dv == var->dv)
> + if (VAR_LOC_DEP_LST (xvar)
> + && dv_as_opaque (VAR_LOC_DEP_LST (xvar)->dv) == dv_as_opaque (var->dv))
> return;
>
> if (var->onepart == NOT_ONEPART)
> @@ -8307,7 +8330,7 @@ notify_dependents_of_resolved_value (variable *ivar,
> variable_table_type *vars)
> continue;
> }
>
> - var = vars->find_with_hash (dv, dv_htab_hash (dv));
> + var = vars->find_with_hash (dv_as_opaque (dv), dv_htab_hash (dv));
>
> if (!var)
> var = variable_from_dropped (dv, NO_INSERT);
> @@ -8552,7 +8575,7 @@ vt_expand_loc_callback (rtx x, bitmap regs,
> return NULL;
> }
>
> - var = elcd->vars->find_with_hash (dv, dv_htab_hash (dv));
> + var = elcd->vars->find_with_hash (dv_as_opaque (dv), dv_htab_hash (dv));
>
> if (!var)
> {
> @@ -8959,8 +8982,9 @@ remove_value_from_changed_variables (rtx val)
> variable **slot;
> variable *var;
>
> - slot = changed_variables->find_slot_with_hash (dv, dv_htab_hash (dv),
> - NO_INSERT);
> + slot = changed_variables->find_slot_with_hash (dv_as_opaque (dv),
> + dv_htab_hash (dv),
> + NO_INSERT);
> var = *slot;
> var->in_changed_variables = false;
> changed_variables->clear_slot (slot);
> @@ -8980,12 +9004,15 @@ notify_dependents_of_changed_value (rtx val,
> variable_table_type *htab,
> loc_exp_dep *led;
> decl_or_value dv = dv_from_rtx (val);
>
> - slot = changed_variables->find_slot_with_hash (dv, dv_htab_hash (dv),
> - NO_INSERT);
> + slot = changed_variables->find_slot_with_hash (dv_as_opaque (dv),
> + dv_htab_hash (dv),
> + NO_INSERT);
> if (!slot)
> - slot = htab->find_slot_with_hash (dv, dv_htab_hash (dv), NO_INSERT);
> + slot = htab->find_slot_with_hash (dv_as_opaque (dv), dv_htab_hash (dv),
> + NO_INSERT);
> if (!slot)
> - slot = dropped_values->find_slot_with_hash (dv, dv_htab_hash (dv),
> + slot = dropped_values->find_slot_with_hash (dv_as_opaque (dv),
> + dv_htab_hash (dv),
> NO_INSERT);
> var = *slot;
>
> @@ -9017,14 +9044,14 @@ notify_dependents_of_changed_value (rtx val,
> variable_table_type *htab,
> break;
>
> case ONEPART_VDECL:
> - ivar = htab->find_with_hash (ldv, dv_htab_hash (ldv));
> + ivar = htab->find_with_hash (dv_as_opaque (ldv), dv_htab_hash (ldv));
> gcc_checking_assert (!VAR_LOC_DEP_LST (ivar));
> variable_was_changed (ivar, NULL);
> break;
>
> case NOT_ONEPART:
> delete led;
> - ivar = htab->find_with_hash (ldv, dv_htab_hash (ldv));
> + ivar = htab->find_with_hash (dv_as_opaque (ldv), dv_htab_hash (ldv));
> if (ivar)
> {
> int i = ivar->n_var_parts;
> @@ -9119,7 +9146,8 @@ emit_notes_for_differences_1 (variable **slot,
> variable_table_type *new_vars)
> variable *old_var, *new_var;
>
> old_var = *slot;
> - new_var = new_vars->find_with_hash (old_var->dv, dv_htab_hash
> (old_var->dv));
> + new_var = new_vars->find_with_hash (dv_as_opaque (old_var->dv),
> + dv_htab_hash (old_var->dv));
>
> if (!new_var)
> {
> @@ -9191,7 +9219,8 @@ emit_notes_for_differences_2 (variable **slot,
> variable_table_type *old_vars)
> variable *old_var, *new_var;
>
> new_var = *slot;
> - old_var = old_vars->find_with_hash (new_var->dv, dv_htab_hash
> (new_var->dv));
> + old_var = old_vars->find_with_hash (dv_as_opaque (new_var->dv),
> + dv_htab_hash (new_var->dv));
> if (!old_var)
> {
> int i;
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
