[v3] Var-Tracking: Typedef pointer_mux<tree_node, rtx_def> as decl_or_value

  From: Pan Li <pan2.li@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.li@intel.com>
Co-Authored-By: Richard Sandiford <richard.sandiford@arm.com>
Co-Authored-By: Richard Biener <rguenther@suse.de>
Co-Authored-By: Jakub Jelinek <jakub@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(-)
  

Thanks, mostly looks good to me.  Some minor comments below.

pan2.li@intel.com writes:
> From: Pan Li <pan2.li@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.li@intel.com>
> Co-Authored-By: Richard Sandiford <richard.sandiford@arm.com>
> Co-Authored-By: Richard Biener <rguenther@suse.de>
> Co-Authored-By: Jakub Jelinek <jakub@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
  

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.sandiford@arm.com> 
Sent: Wednesday, May 10, 2023 11:56 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com; jakub@redhat.com; rguenther@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.li@intel.com writes:
> From: Pan Li <pan2.li@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.li@intel.com>
> Co-Authored-By: Richard Sandiford <richard.sandiford@arm.com>
> Co-Authored-By: Richard Biener <rguenther@suse.de>
> Co-Authored-By: Jakub Jelinek <jakub@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
  

"Li, Pan2" <pan2.li@intel.com> writes:
> 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>'}).

Yeah, since we're adding operator== and operator!= as member operators,
the decl_or_value has to come first.  Please try the conditions in the
order that I'd written them in the review.

Thanks,
Richard
  

Yes, you are right. The decl_or_value take first works well, missed this detail in previous and updated the PATCH v5 for this. Thank you!

Pan

-----Original Message-----
From: Richard Sandiford <richard.sandiford@arm.com> 
Sent: Thursday, May 11, 2023 12:43 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com; jakub@redhat.com; rguenther@suse.de
Subject: Re: [PATCH v3] Var-Tracking: Typedef pointer_mux<tree_node, rtx_def> as decl_or_value

"Li, Pan2" <pan2.li@intel.com> writes:
> 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>'}).

Yeah, since we're adding operator== and operator!= as member operators, the decl_or_value has to come first.  Please try the conditions in the order that I'd written them in the review.

Thanks,
Richard
  

On 11 May 2023 04:30:16 CEST, "Li, Pan2 via Gcc-patches" <gcc-patches@gcc.gnu.org> wrote:

>../../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>'}).

Wouldn't you usually declare operator!= by !(left == right) ?
thanks,