Re: [PATCH 3/4 v2] c++: Use two levels of caching in satisfy_atom

2020-11-06 Thread Jason Merrill via Gcc-patches

On 11/5/20 8:40 PM, Patrick Palka wrote:

This improves the effectiveness of caching in satisfy_atom by querying
the cache again after we've instantiated the atom's parameter mapping.

Before instantiating its mapping, the identity of an (atom,args) pair
within the satisfaction cache is determined by idiosyncratic things like
the level and index of each template parameter used in targets of the
parameter mapping.  For example, the associated constraints of foo in

   template  concept range = range_v;
   template  void foo () requires range && range;

are range_v (with mapping T -> U) /\ range_v (with mapping T -> V).
If during satisfaction the template arguments supplied for U and V are
the same, then the satisfaction value of these two atoms will be the
same (despite their uninstantiated parameter mappings being different).

But sat_cache doesn't see this because it compares the uninstantiated
parameter mapping and the supplied template arguments of sat_entry's
independently.  So satisy_atom on this latter atom will end up fully
evaluating it instead of reusing the satisfaction value of the former.

But there is a point when the two atoms do look the same to sat_cache,
and that's after instantiating their parameter mappings.  By querying
the cache again at this point, we avoid substituting the instantiated
mapping into the second atom's expression.  This in general results in
a higher cache hit rate in satisfy_atom.

With this patch, compile time and memory usage for the cmcstl2 test
test/algorithm/set_symmetric_diference4.cpp drops from 11s/1.4GB to
8.5s/1.2GB with an --enable-checking=release compiler.


OK.


gcc/cp/ChangeLog:

* cp-tree.h (ATOMIC_CONSTR_MAP_INSTANTIATED_P): Define this flag
for ATOMIC_CONSTRs.
* constraint.cc (sat_hasher::hash): Use hash_atomic_constraint
if the flag is set, otherwise keep using a pointer hash.
(sat_hasher::equal): Return false if the flag's setting differs
on two atoms.  Call atomic_constraints_identical_p if the flag
is set, otherwise keep using a pointer equality test.
(satisfy_atom): After instantiating the parameter mapping, form
another ATOMIC_CONSTR using the instantiated mapping and query
the cache again.  Cache the satisfaction value of both atoms.
(diagnose_atomic_constraint): Simplify now that the supplied
atom has an instantiated mapping.
---
  gcc/cp/constraint.cc | 57 +---
  gcc/cp/cp-tree.h |  7 ++
  2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 613ced26e2b..9dd5d892ce9 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -2310,17 +2310,37 @@ struct sat_hasher : ggc_ptr_hash
  {
static hashval_t hash (sat_entry *e)
{
-/* Since normalize_atom caches the ATOMIC_CONSTRs it returns,
-   we can assume pointer-based identity for fast hashing and
-   comparison.  Even if this assumption is violated, that's
-   okay, we'll just get a cache miss.  */
+if (ATOMIC_CONSTR_MAP_INSTANTIATED_P (e->constr))
+  {
+   /* Atoms with instantiated mappings are built during satisfaction.
+  They live only inside the sat_cache, and we build one to query
+  the cache with each time we instantiate a mapping.  */
+   gcc_assert (!e->args);
+   return hash_atomic_constraint (e->constr);
+  }
+
+/* Atoms with uninstantiated mappings are built during normalization.
+   Since normalize_atom caches the atoms it returns, we can assume
+   pointer-based identity for fast hashing and comparison.  Even if this
+   assumption is violated, that's okay, we'll just get a cache miss.  */
  hashval_t value = htab_hash_pointer (e->constr);
+
  return iterative_hash_template_arg (e->args, value);
}
  
static bool equal (sat_entry *e1, sat_entry *e2)

{
-/* As in sat_hasher::hash.  */
+if (ATOMIC_CONSTR_MAP_INSTANTIATED_P (e1->constr)
+   != ATOMIC_CONSTR_MAP_INSTANTIATED_P (e2->constr))
+  return false;
+
+/* See sat_hasher::hash.  */
+if (ATOMIC_CONSTR_MAP_INSTANTIATED_P (e1->constr))
+  {
+   gcc_assert (!e1->args && !e2->args);
+   return atomic_constraints_identical_p (e1->constr, e2->constr);
+  }
+
  if (e1->constr != e2->constr)
return false;
  return template_args_equal (e1->args, e2->args);
@@ -2614,6 +2634,18 @@ satisfy_atom (tree t, tree args, subst_info info)
return cache.save (boolean_false_node);
  }
  
+  /* Now build a new atom using the instantiated mapping.  We use

+ this atom as a second key to the satisfaction cache, and we
+ also pass it to diagnose_atomic_constraint so that diagnostics
+ which refer to the atom display the instantiated mapping.  */
+  t = copy_node (t);
+  ATOMIC_CONSTR_MAP (t) = map;
+  gcc_assert (!ATOMIC_CONSTR_MAP_INSTANTIATED_P (t));
+  

[PATCH 3/4 v2] c++: Use two levels of caching in satisfy_atom

2020-11-05 Thread Patrick Palka via Gcc-patches
This improves the effectiveness of caching in satisfy_atom by querying
the cache again after we've instantiated the atom's parameter mapping.

Before instantiating its mapping, the identity of an (atom,args) pair
within the satisfaction cache is determined by idiosyncratic things like
the level and index of each template parameter used in targets of the
parameter mapping.  For example, the associated constraints of foo in

  template  concept range = range_v;
  template  void foo () requires range && range;

are range_v (with mapping T -> U) /\ range_v (with mapping T -> V).
If during satisfaction the template arguments supplied for U and V are
the same, then the satisfaction value of these two atoms will be the
same (despite their uninstantiated parameter mappings being different).

But sat_cache doesn't see this because it compares the uninstantiated
parameter mapping and the supplied template arguments of sat_entry's
independently.  So satisy_atom on this latter atom will end up fully
evaluating it instead of reusing the satisfaction value of the former.

But there is a point when the two atoms do look the same to sat_cache,
and that's after instantiating their parameter mappings.  By querying
the cache again at this point, we avoid substituting the instantiated
mapping into the second atom's expression.  This in general results in
a higher cache hit rate in satisfy_atom.

With this patch, compile time and memory usage for the cmcstl2 test
test/algorithm/set_symmetric_diference4.cpp drops from 11s/1.4GB to
8.5s/1.2GB with an --enable-checking=release compiler.

gcc/cp/ChangeLog:

* cp-tree.h (ATOMIC_CONSTR_MAP_INSTANTIATED_P): Define this flag
for ATOMIC_CONSTRs.
* constraint.cc (sat_hasher::hash): Use hash_atomic_constraint
if the flag is set, otherwise keep using a pointer hash.
(sat_hasher::equal): Return false if the flag's setting differs
on two atoms.  Call atomic_constraints_identical_p if the flag
is set, otherwise keep using a pointer equality test.
(satisfy_atom): After instantiating the parameter mapping, form
another ATOMIC_CONSTR using the instantiated mapping and query
the cache again.  Cache the satisfaction value of both atoms.
(diagnose_atomic_constraint): Simplify now that the supplied
atom has an instantiated mapping.
---
 gcc/cp/constraint.cc | 57 +---
 gcc/cp/cp-tree.h |  7 ++
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/gcc/cp/constraint.cc b/gcc/cp/constraint.cc
index 613ced26e2b..9dd5d892ce9 100644
--- a/gcc/cp/constraint.cc
+++ b/gcc/cp/constraint.cc
@@ -2310,17 +2310,37 @@ struct sat_hasher : ggc_ptr_hash
 {
   static hashval_t hash (sat_entry *e)
   {
-/* Since normalize_atom caches the ATOMIC_CONSTRs it returns,
-   we can assume pointer-based identity for fast hashing and
-   comparison.  Even if this assumption is violated, that's
-   okay, we'll just get a cache miss.  */
+if (ATOMIC_CONSTR_MAP_INSTANTIATED_P (e->constr))
+  {
+   /* Atoms with instantiated mappings are built during satisfaction.
+  They live only inside the sat_cache, and we build one to query
+  the cache with each time we instantiate a mapping.  */
+   gcc_assert (!e->args);
+   return hash_atomic_constraint (e->constr);
+  }
+
+/* Atoms with uninstantiated mappings are built during normalization.
+   Since normalize_atom caches the atoms it returns, we can assume
+   pointer-based identity for fast hashing and comparison.  Even if this
+   assumption is violated, that's okay, we'll just get a cache miss.  */
 hashval_t value = htab_hash_pointer (e->constr);
+
 return iterative_hash_template_arg (e->args, value);
   }
 
   static bool equal (sat_entry *e1, sat_entry *e2)
   {
-/* As in sat_hasher::hash.  */
+if (ATOMIC_CONSTR_MAP_INSTANTIATED_P (e1->constr)
+   != ATOMIC_CONSTR_MAP_INSTANTIATED_P (e2->constr))
+  return false;
+
+/* See sat_hasher::hash.  */
+if (ATOMIC_CONSTR_MAP_INSTANTIATED_P (e1->constr))
+  {
+   gcc_assert (!e1->args && !e2->args);
+   return atomic_constraints_identical_p (e1->constr, e2->constr);
+  }
+
 if (e1->constr != e2->constr)
   return false;
 return template_args_equal (e1->args, e2->args);
@@ -2614,6 +2634,18 @@ satisfy_atom (tree t, tree args, subst_info info)
   return cache.save (boolean_false_node);
 }
 
+  /* Now build a new atom using the instantiated mapping.  We use
+ this atom as a second key to the satisfaction cache, and we
+ also pass it to diagnose_atomic_constraint so that diagnostics
+ which refer to the atom display the instantiated mapping.  */
+  t = copy_node (t);
+  ATOMIC_CONSTR_MAP (t) = map;
+  gcc_assert (!ATOMIC_CONSTR_MAP_INSTANTIATED_P (t));
+  ATOMIC_CONSTR_MAP_INSTANTIATED_P (t) = true;
+  satisfaction_cache inst_cache (t,