RE: [PATCH v3] Var-Tracking: Typedef pointer_mux as decl_or_value
On 11 May 2023 04:30:16 CEST, "Li, Pan2 via Gcc-patches"
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'}).
Wouldn't you usually declare operator!= by !(left == right) ?
thanks,
RE: [PATCH v3] Var-Tracking: Typedef pointer_mux as decl_or_value
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
Sent: Thursday, May 11, 2023 12:43 PM
To: Li, Pan2
Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang,
Yanzhang ; jeffreya...@gmail.com; ja...@redhat.com;
rguent...@suse.de
Subject: Re: [PATCH v3] Var-Tracking: Typedef pointer_mux
as decl_or_value
"Li, Pan2" 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'}).
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
Re: [PATCH v3] Var-Tracking: Typedef pointer_mux as decl_or_value
"Li, Pan2" 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'}).
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
RE: [PATCH v3] Var-Tracking: Typedef pointer_mux as decl_or_value
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'}).
Pan
-Original Message-
From: Richard Sandiford
Sent: Wednesday, May 10, 2023 11:56 PM
To: Li, Pan2
Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang,
Yanzhang ; jeffreya...@gmail.com; ja...@redhat.com;
rguent...@suse.de
Subject: Re: [PATCH v3] Var-Tracking: Typedef pointer_mux
as decl_or_value
Thanks, mostly looks good to me. Some minor comments below.
pan2...@intel.com writes:
> From: Pan Li
>
> 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
> Co-Authored-By: Richard Sandiford
> Co-Authored-By: Richard Biener
> Co-Authored-By: Jakub Jelinek
>
> 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.
> (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 ) const
> +{
> + return this->m_ptr == other.m_ptr;
> +}
> +
> + // Return true if class has the different m_ptr, or false.
> + bool operator != (const pointer_mux ) 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.
Re: [PATCH v3] Var-Tracking: Typedef pointer_mux as decl_or_value
Thanks, mostly looks good to me. Some minor comments below.
pan2...@intel.com writes:
> From: Pan Li
>
> 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
> Co-Authored-By: Richard Sandiford
> Co-Authored-By: Richard Biener
> Co-Authored-By: Jakub Jelinek
>
> 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.
> (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 ) const
> +{
> + return this->m_ptr == other.m_ptr;
> +}
> +
> + // Return true if class has the different m_ptr, or false.
> + bool operator != (const pointer_mux ) 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 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 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
