RE: [PATCH] Var-Tracking: Leverage pointer_mux for decl_or_value

["Li, Pan2" <pan2.li@intel.com>]

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