I see, will try to get rid of dv_as_opaque everywhere. Thank you all!
Pan
-----Original Message-----
From: Richard Sandiford <richard.sandif...@arm.com>
Sent: Wednesday, May 10, 2023 8:53 PM
To: Jakub Jelinek <ja...@redhat.com>
Cc: Li, Pan2 <pan2...@intel.com>; 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
Subject: Re: [PATCH v2] Var-Tracking: Typedef pointer_mux<tree_node, rtx_def>
as decl_or_value
Jakub Jelinek <ja...@redhat.com> writes:
> On Wed, May 10, 2023 at 07:57:05PM +0800, pan2...@intel.com wrote:
>> --- a/gcc/var-tracking.cc
>> +++ b/gcc/var-tracking.cc
>> @@ -116,9 +116,14 @@
>> #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;
>> +
>> /* 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 +201,11 @@ struct micro_operation };
>>
>>
>> -/* A declaration of a variable, or an RTL value being handled like a
>> - declaration. */
>> -typedef void *decl_or_value;
>
> Why do you move the typedef?
>
>> @@ -503,9 +505,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;
>> }
>
> I'm not convinced this is correct. I think all the
> find_slot_with_hash etc. pass in a decl_or_value, so I'd expect y to
> have decl_or_value type or something similar.
>
>> /* Free the element of VARIABLE_HTAB (its type is struct
>> variable_def). */ @@ -1396,8 +1396,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::first (decl);
>
> Can't you just decl_or_value dv = decl; ? I think pointer_mux has
> ctors from pointers to the template parameter types.
>
>> gcc_checking_assert (dv_is_decl_p (dv));
>> return dv;
>> }
>> @@ -1406,8 +1405,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::second (value);
>
> Ditto.
>
>> @@ -1661,7 +1659,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);
>
> Then you wouldn't need to change all these.
Also, please do try changing variable_hasher::compare_type to decl_or_value,
and changing the type of the second parameter to variable_hasher::equal
accordingly. I still feel that we should be able to get rid of dv_as_opaque
entirely.
Thanks,
Richard
