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 <ja...@redhat.com> Sent: Wednesday, May 10, 2023 4:14 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; rguent...@suse.de; richard.sandif...@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
