Richard Biener <rguent...@suse.de> writes:
> On Wed, 10 May 2023, pan2...@intel.com wrote:
>
>> From: Pan Li <pan2...@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...@intel.com>
>> Co-Authored-By: Richard Sandiford <richard.sandif...@arm.com>
>> Co-Authored-By: Richard Biener <rguent...@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
