Re: [PATCH] Implement ipa_vr hashing.

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Mon, May 22 2023, Aldy Hernandez via Gcc-patches wrote:
> Implement hashing for ipa_vr.  When all is said and done, all these
> patches incurr a 7.64% slowdown for ipa-cp, with is entirely covered by
> the similar 7% increase in this area last week.  So we get type agnostic
> ranges with "infinite" range precision close to free.
>
> There is no change in overall compilation.
>
> OK?
>

One small request....

> gcc/ChangeLog:
>
> 	* ipa-prop.cc (struct ipa_vr_ggc_hash_traits): Adjust for use with
> 	ipa_vr instead of value_range.
> 	(gt_pch_nx): Same.
> 	(gt_ggc_mx): Same.
> 	(ipa_get_value_range): Same.
> 	* value-range.cc (gt_pch_nx): Move to ipa-prop.cc and adjust for
> 	ipa_vr.
> 	(gt_ggc_mx): Same.
> ---
>  gcc/ipa-prop.cc    | 76 +++++++++++++++++++++++++++-------------------
>  gcc/value-range.cc | 15 ---------
>  2 files changed, 45 insertions(+), 46 deletions(-)
>
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index c46a89f1b49..6383bc11e0a 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -109,53 +109,53 @@ struct ipa_bit_ggc_hash_traits : public ggc_cache_remove <ipa_bits *>
>  /* Hash table for avoid repeated allocations of equal ipa_bits.  */
>  static GTY ((cache)) hash_table<ipa_bit_ggc_hash_traits> *ipa_bits_hash_table;
>  
> -/* Traits for a hash table for reusing value_ranges used for IPA.  Note that
> -   the equiv bitmap is not hashed and is expected to be NULL.  */
> +/* Traits for a hash table for reusing ranges.  */
>  
> -struct ipa_vr_ggc_hash_traits : public ggc_cache_remove <value_range *>
> +struct ipa_vr_ggc_hash_traits : public ggc_cache_remove <ipa_vr *>
>  {
> -  typedef value_range *value_type;
> -  typedef value_range *compare_type;
> +  typedef ipa_vr *value_type;
> +  typedef const vrange *compare_type;
>    static hashval_t
> -  hash (const value_range *p)
> +  hash (const ipa_vr *p)
>      {
> -      tree min, max;
> -      value_range_kind kind = get_legacy_range (*p, min, max);
> -      inchash::hash hstate (kind);
> -      inchash::add_expr (min, hstate);
> -      inchash::add_expr (max, hstate);
> +      // This never get called, except in the verification code, as
> +      // ipa_get_value_range() calculates the hash itself.  This
> +      // function is mostly here for completness' sake.
> +      Value_Range vr;
> +      p->get_vrange (vr);
> +      inchash::hash hstate;
> +      add_vrange (vr, hstate);
>        return hstate.end ();
>      }
>    static bool
> -  equal (const value_range *a, const value_range *b)
> +  equal (const ipa_vr *a, const vrange *b)
>      {
> -      return (types_compatible_p (a->type (), b->type ())
> -	      && *a == *b);
> +      return a->equal_p (*b);
>      }
>    static const bool empty_zero_p = true;
>    static void
> -  mark_empty (value_range *&p)
> +  mark_empty (ipa_vr *&p)
>      {
>        p = NULL;
>      }
>    static bool
> -  is_empty (const value_range *p)
> +  is_empty (const ipa_vr *p)
>      {
>        return p == NULL;
>      }
>    static bool
> -  is_deleted (const value_range *p)
> +  is_deleted (const ipa_vr *p)
>      {
> -      return p == reinterpret_cast<const value_range *> (1);
> +      return p == reinterpret_cast<const ipa_vr *> (1);
>      }
>    static void
> -  mark_deleted (value_range *&p)
> +  mark_deleted (ipa_vr *&p)
>      {
> -      p = reinterpret_cast<value_range *> (1);
> +      p = reinterpret_cast<ipa_vr *> (1);
>      }
>  };
>  
> -/* Hash table for avoid repeated allocations of equal value_ranges.  */
> +/* Hash table for avoid repeated allocations of equal ranges.  */
>  static GTY ((cache)) hash_table<ipa_vr_ggc_hash_traits> *ipa_vr_hash_table;
>  
>  /* Holders of ipa cgraph hooks: */
> @@ -265,6 +265,22 @@ ipa_vr::dump (FILE *out) const
>      fprintf (out, "NO RANGE");
>  }
>  
> +// ?? These stubs are because we use an ipa_vr in a hash_traits and
> +// hash-traits.h defines an extern of gt_ggc_mx (T &) instead of
> +// picking up the gt_ggc_mx (T *) version.

If you mean FIXME or TODO, please replace the "??" string with one of
those.  Otherwise please just remove it or specify what you mean in some
clearer way.

OK with that change.

Thanks,

Martin



> +void
> +gt_pch_nx (ipa_vr *&x)
> +{
> +  return gt_pch_nx ((ipa_vr *) x);
> +}
> +
> +void
> +gt_ggc_mx (ipa_vr *&x)
> +{
> +  return gt_ggc_mx ((ipa_vr *) x);
> +}
> +
> +

[...]