Thanks Richard Sandiford. Update PATCH v4 here ->
https://gcc.gnu.org/pipermail/gcc-patches/2023-May/618099.html.
> - if (dv_as_opaque (node->dv) != decl || node->offset != offset)
> + if (node->dv.first_or_null () != decl || node->offset !=
> + offset)
> Genuine question, but: is the first_or_null really needed? I would have
> expected node->dv != decl to work, with an implicit conversion on the
> argument.
Directly compare node->dv and decl may requires additional overload operator,
or it may complains similar as below. But I am afraid it is unreasonable to add
such kind of operator for one specific type RTX in pointer_mux up to a point.
Thus I think here we may need node->dv == (decl_or_val) decl here.
../../gcc/var-tracking.cc:3233:28: error: no match for 'operator!=' (operand
types are 'rtx' {aka 'rtx_def*'} and 'decl_or_value' {aka
'pointer_mux<tree_node, rtx_def>'}).
Pan
-----Original Message-----
From: Richard Sandiford <richard.sandif...@arm.com>
Sent: Wednesday, May 10, 2023 11:56 PM
To: Li, Pan2 <pan2...@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang,
Yanzhang <yanzhang.w...@intel.com>; jeffreya...@gmail.com; ja...@redhat.com;
rguent...@suse.de
Subject: Re: [PATCH v3] Var-Tracking: Typedef pointer_mux<tree_node, rtx_def>
as decl_or_value
Thanks, mostly looks good to me. Some minor comments below.
pan2...@intel.com writes:
> From: Pan Li <pan2...@intel.com>
>
> 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 <pan2...@intel.com>
> Co-Authored-By: Richard Sandiford <richard.sandif...@arm.com>
> Co-Authored-By: Richard Biener <rguent...@suse.de>
> Co-Authored-By: Jakub Jelinek <ja...@redhat.com>
>
> gcc/ChangeLog:
>
> * mux-utils.h: Add overload operator == and != for pointer_mux.
> * var-tracking.cc: Included mux-utils.h for pointer_tmux.
> (decl_or_value): Changed from void * to pointer_mux<tree_node, rtx_def>.
> (dv_is_decl_p): Reconciled to the new type, aka pointer_mux.
> (dv_as_decl): Ditto.
> (dv_as_opaque): Removed due to unnecessary.
> (struct variable_hasher): Take decl_or_value as compare_type.
> (variable_hasher::equal): Diito.
> (dv_from_decl): Reconciled to the new type, aka pointer_mux.
> (dv_from_value): Ditto.
> (attrs_list_member): Ditto.
> (vars_copy): Ditto.
> (var_reg_decl_set): Ditto.
> (var_reg_delete_and_set): Ditto.
> (find_loc_in_1pdv): Ditto.
> (canonicalize_values_star): Ditto.
> (variable_post_merge_new_vals): Ditto.
> (dump_onepart_variable_differences): Ditto.
> (variable_different_p): Ditto.
> (variable_was_changed): Ditto.
> (set_slot_part): Ditto.
> (clobber_slot_part): Ditto.
> (clobber_variable_part): Ditto.
> (remove_value_from_changed_variables): Ditto.
> (notify_dependents_of_changed_value): Ditto.
> ---
> gcc/mux-utils.h | 12 ++++++
> gcc/var-tracking.cc | 96
> ++++++++++++++++++---------------------------
> 2 files changed, 51 insertions(+), 57 deletions(-)
>
> diff --git a/gcc/mux-utils.h b/gcc/mux-utils.h index
> a2b6a316899..adf3d3b722b 100644
> --- a/gcc/mux-utils.h
> +++ b/gcc/mux-utils.h
> @@ -72,6 +72,18 @@ public:
> // Return true unless the pointer is a null A pointer.
> explicit operator bool () const { return m_ptr; }
>
> + // Return true if class has the same m_ptr, or false.
> + bool operator == (const pointer_mux &other) const
> + {
> + return this->m_ptr == other.m_ptr;
> + }
> +
> + // Return true if class has the different m_ptr, or false.
> + bool operator != (const pointer_mux &other) const
> + {
> + return this->m_ptr != other.m_ptr;
> + }
> +
The current code tries to follow the coding standard rule that functions should
be defined outside the class if the whole thing doesn't fit on one line.
Admittedly that's not widely followed, but we might as well continue to stick
to it here.
The comment shouldn't talk about m_ptr, since that's an internal implementation
detail rather than a user-facing thing. I think it's OK to leave the functions
uncommented, since it's obvious what == and != do.
> // Assign A and B pointers respectively.
> void set_first (T1 *ptr) { *this = first (ptr); }
> void set_second (T2 *ptr) { *this = second (ptr); } diff --git
> a/gcc/var-tracking.cc b/gcc/var-tracking.cc index
> fae0c73e02f..7a35f49020a 100644
> --- a/gcc/var-tracking.cc
> +++ b/gcc/var-tracking.cc
> @@ -116,6 +116,7 @@
> #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;
>
> @@ -197,14 +198,14 @@ struct micro_operation
>
>
> /* A declaration of a variable, or an RTL value being handled like a
> - declaration. */
> -typedef void *decl_or_value;
> + declaration by pointer_mux. */
> +typedef pointer_mux<tree_node, rtx_def> 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;
> + return dv.is_first ();
> }
>
> /* Return true if a decl_or_value is a VALUE rtl. */ @@ -219,7
> +220,7 @@ static inline tree dv_as_decl (decl_or_value dv) {
> gcc_checking_assert (dv_is_decl_p (dv));
> - return (tree) dv;
> + return dv.known_first ();
> }
>
> /* Return the value in the decl_or_value. */ @@ -227,14 +228,7 @@
> static inline rtx dv_as_value (decl_or_value dv) {
> gcc_checking_assert (dv_is_value_p (dv));
> - return (rtx)dv;
> -}
> -
> -/* Return the opaque pointer in the decl_or_value. */ -static inline
> void * -dv_as_opaque (decl_or_value dv) -{
> - return dv;
> + return dv.known_second ();
> }
>
>
> @@ -483,9 +477,9 @@ static void variable_htab_free (void *);
>
> struct variable_hasher : pointer_hash <variable> {
> - typedef void *compare_type;
> + typedef decl_or_value compare_type;
> static inline hashval_t hash (const variable *);
> - static inline bool equal (const variable *, const void *);
> + static inline bool equal (const variable *, const decl_or_value);
> static inline void remove (variable *); };
>
> @@ -501,11 +495,9 @@ variable_hasher::hash (const variable *v)
> /* Compare the declaration of variable X with declaration Y. */
>
> inline bool
> -variable_hasher::equal (const variable *v, const void *y)
> +variable_hasher::equal (const variable *v, const decl_or_value y)
> {
> - decl_or_value dv = CONST_CAST2 (decl_or_value, const void *, y);
> -
> - return (dv_as_opaque (v->dv) == dv_as_opaque (dv));
> + return v->dv == y;
> }
>
> /* Free the element of VARIABLE_HTAB (its type is struct
> variable_def). */ @@ -1396,8 +1388,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;
> gcc_checking_assert (dv_is_decl_p (dv));
> return dv;
> }
> @@ -1406,8 +1397,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 = value;
> gcc_checking_assert (dv_is_value_p (dv));
> return dv;
> }
> @@ -1519,7 +1509,7 @@ static attrs *
> attrs_list_member (attrs *list, decl_or_value dv, HOST_WIDE_INT
> offset) {
> for (; list; list = list->next)
> - if (dv_as_opaque (list->dv) == dv_as_opaque (dv) && list->offset ==
> offset)
> + if (list->dv == dv && list->offset == offset)
> return list;
> return NULL;
> }
> @@ -1831,8 +1821,7 @@ 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 (var->dv, dv_htab_hash
> + (var->dv), INSERT);
> *dstp = var;
> }
> }
> @@ -1866,8 +1855,7 @@ var_reg_decl_set (dataflow_set *set, rtx loc, enum
> var_init_status initialized,
> dv = dv_from_decl (var_debug_decl (dv_as_decl (dv)));
>
> for (node = set->regs[REGNO (loc)]; node; node = node->next)
> - if (dv_as_opaque (node->dv) == dv_as_opaque (dv)
> - && node->offset == offset)
> + if (node->dv == dv && node->offset == offset)
> break;
> if (!node)
> attrs_list_insert (&set->regs[REGNO (loc)], dv, offset, loc); @@
> -1966,7 +1954,7 @@ var_reg_delete_and_set (dataflow_set *set, rtx loc, bool
> modify,
> for (node = *nextp; node; node = next)
> {
> next = node->next;
> - if (dv_as_opaque (node->dv) != decl || node->offset != offset)
> + if (node->dv.first_or_null () != decl || node->offset !=
> + offset)
Genuine question, but: is the first_or_null really needed? I would have
expected node->dv != decl to work, with an implicit conversion on the argument.
> {
> delete_variable_part (set, node->loc, node->dv, node->offset);
> delete node;
> @@ -3242,7 +3230,7 @@ find_loc_in_1pdv (rtx loc, variable *var,
> variable_table_type *vars)
> if (!var->n_var_parts)
> return NULL;
>
> - gcc_checking_assert (loc != dv_as_opaque (var->dv));
> + gcc_checking_assert (loc != var->dv.second_or_null ());
Similarly here, I would expect var->dv != loc to work.
> loc_code = GET_CODE (loc);
> for (node = var->var_part[0].loc_chain; node; node = node->next) @@
> -3832,16 +3820,14 @@ canonicalize_values_star (variable **slot,
> dataflow_set *set)
>
> while (list)
> {
> - if (list->offset == 0
> - && (dv_as_opaque (list->dv) == dv_as_opaque (dv)
> - || dv_as_opaque (list->dv) == dv_as_opaque (cdv)))
> + if (list->offset == 0 && (list->dv == dv || list->dv == cdv))
> break;
>
> list = list->next;
> }
>
> gcc_assert (list);
> - if (dv_as_opaque (list->dv) == dv_as_opaque (dv))
> + if (list->dv == dv)
> {
> list->dv = cdv;
> for (listp = &list->next; (list = *listp); listp = &list->next)
> @@
> -3849,7 +3835,7 @@ canonicalize_values_star (variable **slot, dataflow_set
> *set)
> if (list->offset)
> continue;
>
> - if (dv_as_opaque (list->dv) == dv_as_opaque (cdv))
> + if (list->dv == cdv)
> {
> *listp = list->next;
> delete list;
> @@ -3857,17 +3843,17 @@ canonicalize_values_star (variable **slot,
> dataflow_set *set)
> break;
> }
>
> - gcc_assert (dv_as_opaque (list->dv) != dv_as_opaque (dv));
> + gcc_assert (list->dv != dv);
> }
> }
> - else if (dv_as_opaque (list->dv) == dv_as_opaque (cdv))
> + else if (list->dv == cdv)
> {
> for (listp = &list->next; (list = *listp); listp = &list->next)
> {
> if (list->offset)
> continue;
>
> - if (dv_as_opaque (list->dv) == dv_as_opaque (dv))
> + if (list->dv == dv)
> {
> *listp = list->next;
> delete list;
> @@ -3875,7 +3861,7 @@ canonicalize_values_star (variable **slot, dataflow_set
> *set)
> break;
> }
>
> - gcc_assert (dv_as_opaque (list->dv) != dv_as_opaque (cdv));
> + gcc_assert (list->dv != cdv);
> }
> }
> else
> @@ -3884,9 +3870,7 @@ canonicalize_values_star (variable **slot, dataflow_set
> *set)
> if (flag_checking)
> while (list)
> {
> - if (list->offset == 0
> - && (dv_as_opaque (list->dv) == dv_as_opaque (dv)
> - || dv_as_opaque (list->dv) == dv_as_opaque (cdv)))
> + if (list->offset == 0 && (list->dv == dv || list->dv == cdv))
> gcc_unreachable ();
>
> list = list->next;
> @@ -4475,7 +4459,7 @@ variable_post_merge_new_vals (variable **slot,
> dfset_post_merge *dfpm)
> check_dupes = true;
> break;
> }
> - else if (dv_as_opaque (att->dv) == dv_as_opaque (var->dv))
> + else if (att->dv == var->dv)
> curp = attp;
> }
>
> @@ -4485,7 +4469,7 @@ variable_post_merge_new_vals (variable **slot,
> dfset_post_merge *dfpm)
> while (*curp)
> if ((*curp)->offset == 0
> && GET_MODE ((*curp)->loc) == GET_MODE (node->loc)
> - && dv_as_opaque ((*curp)->dv) == dv_as_opaque (var->dv))
> + && (*curp)->dv == var->dv)
> break;
> else
> curp = &(*curp)->next;
> @@ -4989,7 +4973,7 @@ dump_onepart_variable_differences (variable
> *var1, variable *var2)
>
> gcc_assert (var1 != var2);
> gcc_assert (dump_file);
> - gcc_assert (dv_as_opaque (var1->dv) == dv_as_opaque (var2->dv));
> + gcc_assert (var1->dv == var2->dv);
> gcc_assert (var1->n_var_parts == 1
> && var2->n_var_parts == 1);
>
> @@ -5054,8 +5038,7 @@ variable_different_p (variable *var1, variable
> *var2)
>
> if (var1->onepart && var1->n_var_parts)
> {
> - gcc_checking_assert (dv_as_opaque (var1->dv) == dv_as_opaque (var2->dv)
> - && var1->n_var_parts == 1);
> + gcc_checking_assert (var1->dv == var2->dv && var1->n_var_parts
> + == 1);
> /* One-part values have locations in a canonical order. */
> return onepart_variable_different_p (var1, var2);
> }
> @@ -7520,9 +7503,10 @@ 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 (var->dv,
> + dv_htab_hash (var->dv),
> + INSERT);
This is now a no-op change, so probably best to leave it out.
> empty_var = *dslot;
>
> if (empty_var)
> @@ -7656,7 +7640,7 @@ set_slot_part (dataflow_set *set, rtx loc, variable
> **slot,
> onepart = dv_onepart_p (dv);
>
> gcc_checking_assert (offset == 0 || !onepart);
> - gcc_checking_assert (loc != dv_as_opaque (dv));
> + gcc_checking_assert (loc != dv.second_or_null ());
dv != loc here too.
>
> if (! flag_var_tracking_uninit)
> initialized = VAR_INIT_STATUS_INITIALIZED; @@ -7684,7 +7668,7 @@
> set_slot_part (dataflow_set *set, rtx loc, variable **slot,
> {
> int r = -1, c = 0;
>
> - gcc_assert (dv_as_opaque (var->dv) == dv_as_opaque (dv));
> + gcc_assert (var->dv == dv);
>
> pos = 0;
>
> @@ -7950,8 +7934,7 @@ clobber_slot_part (dataflow_set *set, rtx loc, variable
> **slot,
> for (anode = *anextp; anode; anode = anext)
> {
> anext = anode->next;
> - if (dv_as_opaque (anode->dv) == dv_as_opaque (var->dv)
> - && anode->offset == offset)
> + if (anode->dv == var->dv && anode->offset == offset)
> {
> delete anode;
> *anextp = anext;
> @@ -7980,8 +7963,7 @@ clobber_variable_part (dataflow_set *set, rtx
> loc, decl_or_value dv, {
> variable **slot;
>
> - if (!dv_as_opaque (dv)
> - || (!dv_is_value_p (dv) && ! DECL_P (dv_as_decl (dv))))
> + if (!dv || (!dv_is_value_p (dv) && ! DECL_P (dv_as_decl (dv))))
> return;
>
> slot = shared_hash_find_slot_noinsert (set->vars, dv); @@ -8960,7
> +8942,7 @@ remove_value_from_changed_variables (rtx val)
> variable *var;
>
> slot = changed_variables->find_slot_with_hash (dv, dv_htab_hash (dv),
> - NO_INSERT);
> + NO_INSERT);
> var = *slot;
> var->in_changed_variables = false;
> changed_variables->clear_slot (slot); @@ -8981,7 +8963,7 @@
> notify_dependents_of_changed_value (rtx val, variable_table_type *htab,
> decl_or_value dv = dv_from_rtx (val);
>
> slot = changed_variables->find_slot_with_hash (dv, dv_htab_hash (dv),
> - NO_INSERT);
> + NO_INSERT);
> if (!slot)
> slot = htab->find_slot_with_hash (dv, dv_htab_hash (dv), NO_INSERT);
> if (!slot)
The last two changes are no-ops.
Thanks,
Richard
