Var-Tracking: Leverage pointer_mux for 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>

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(-)
  

On Wed, 10 May 2023, pan2.li@intel.com wrote:

> 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>
> 
> 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 <rguenther@suse.de> writes:
> On Wed, 10 May 2023, pan2.li@intel.com wrote:
>
>> 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>
>> 
>> 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

Was just about to say the same thing :)

Admittedly, I think I might be missing some of the intended abstraction
here, but at least for variable_table_type, it seems that the void *
compare_type in:

struct variable_hasher : pointer_hash <variable>
{
  typedef void *compare_type;
  static inline hashval_t hash (const variable *);
  static inline bool equal (const variable *, const void *);
  static inline void remove (variable *);
};

could/should really be decl_or_value, even under the current scheme.
Perhaps the current code predates strongly-typed hash containers.

> - 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)?

Would prefer ==/!= for now.  Kind-of wary of providing access to the
underlying representation.

Thanks,
Richard
  

On Wed, May 10, 2023 at 03:17:58PM +0800, Pan Li via Gcc-patches wrote:
> gcc/ChangeLog:
> 
> 	* var-tracking.cc (DECL_OR_VALUE_OR_DEFAULT): New macro for
> 	  clean code.

ChangeLog formatting shouldn't have spaces after the initial tab.
Furthermore, the entry doesn't describe what changes you've made.
It should start with:
	* var-tracking.cc: Include mux-utils.h.
	(decl_or_value): Changed from void * to
	pointer_mux<tree_node, rtx_def>.
	(DECL_OR_VALUE_OR_DEFAULT): Define.
etc.

> --- 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;

I really don't understand why it needs to be so complicated.
decl_or_value is dv_is_decl_p if it is NULL or if it is a tree,
and is false if it is rtx VALUE, no other rtxes are expected.
pointer_mux<tree_node, rtx_def> should accept nullptr as being the first
one, so i'd expect just

/* 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.is_first ();
}

/* Return true if a decl_or_value is a VALUE rtl.  */
static inline bool
dv_is_value_p (decl_or_value dv)
{
  return dv.is_second ();
} 

/* Return the decl in the decl_or_value.  */
static inline tree
dv_as_decl (decl_or_value dv)
{
  gcc_checking_assert (dv_is_decl_p (dv));
  return dv.known_first ();
}
  
/* Return the value in the decl_or_value.  */
static inline rtx
dv_as_value (decl_or_value dv)
{
  gcc_checking_assert (dv_is_value_p (dv));
  return dv.known_second ();
}
   
/* Return the opaque pointer in the decl_or_value.  */
static inline void *
dv_as_opaque (decl_or_value dv)
{
  return dv.is_first () ? (void *) dv.known_first ()
			: (void *) dv.known_second ();
}

// Ideally dv_as_opaque would just return m_ptr but that
// is unfortunately private.

And define a hasher for decl_or_value now that it is a class
(that would hash/compare the m_ptr value or separately
dv.is_first () bool and dv_as_opaque pointer).

And then I'd hope you don't need to do any further changes
in the file.

	Jakub
  

Thanks all for comments.

Looks like pay too much attention for the NULL check but it is covered by pointer_mux already. Update PATCH v2 as below, please help to review continuously.

https://gcc.gnu.org/pipermail/gcc-patches/2023-May/618007.html

Pan

-----Original Message-----
From: Jakub Jelinek <jakub@redhat.com> 
Sent: Wednesday, May 10, 2023 4:14 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; rguenther@suse.de; richard.sandiford@arm.com
Subject: Re: [PATCH] Var-Tracking: Leverage pointer_mux for decl_or_value

On Wed, May 10, 2023 at 03:17:58PM +0800, Pan Li via Gcc-patches wrote:
> gcc/ChangeLog:
> 
> 	* var-tracking.cc (DECL_OR_VALUE_OR_DEFAULT): New macro for
> 	  clean code.

ChangeLog formatting shouldn't have spaces after the initial tab.
Furthermore, the entry doesn't describe what changes you've made.
It should start with:
	* var-tracking.cc: Include mux-utils.h.
	(decl_or_value): Changed from void * to
	pointer_mux<tree_node, rtx_def>.
	(DECL_OR_VALUE_OR_DEFAULT): Define.
etc.

> --- 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;

I really don't understand why it needs to be so complicated.
decl_or_value is dv_is_decl_p if it is NULL or if it is a tree, and is false if it is rtx VALUE, no other rtxes are expected.
pointer_mux<tree_node, rtx_def> should accept nullptr as being the first one, so i'd expect just

/* 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.is_first ();
}

/* Return true if a decl_or_value is a VALUE rtl.  */ static inline bool dv_is_value_p (decl_or_value dv) {
  return dv.is_second ();
} 

/* Return the decl in the decl_or_value.  */ static inline tree dv_as_decl (decl_or_value dv) {
  gcc_checking_assert (dv_is_decl_p (dv));
  return dv.known_first ();
}
  
/* Return the value in the decl_or_value.  */ static inline rtx dv_as_value (decl_or_value dv) {
  gcc_checking_assert (dv_is_value_p (dv));
  return dv.known_second ();
}
   
/* Return the opaque pointer in the decl_or_value.  */ static inline void * dv_as_opaque (decl_or_value dv) {
  return dv.is_first () ? (void *) dv.known_first ()
			: (void *) dv.known_second ();
}

// Ideally dv_as_opaque would just return m_ptr but that // is unfortunately private.

And define a hasher for decl_or_value now that it is a class (that would hash/compare the m_ptr value or separately dv.is_first () bool and dv_as_opaque pointer).

And then I'd hope you don't need to do any further changes in the file.

	Jakub