diff mbox series

Fix handling of access ranges in ipa-modref

Message ID 20201011122115.GA98856@kam.mff.cuni.cz
State New
Headers show
Series Fix handling of access ranges in ipa-modref | expand

Commit Message

Jan Hubicka Oct. 11, 2020, 12:21 p.m. UTC 
Hi,
this patch fixes the range tracking in argument and re-enables it for clones
(the bug that broke dealII and x264 benchmarks)

It turned out that there was three problems
 1) for SRA/ipa-cp clones we did not update summarries to represent new
    signature.  This is now done in modref_transform.
    I tested it in ipa-sra testcases and it seems to work fine, but Martin,
    can you please take a look?

    Param adjustment interface provides original indexes for new indexes of
    parameters.  I need reverse information: for original index I need new
    that I compute via array map and then do the rewrite.

    Martin, if things passed by references are translated to stuff
    passed by value, we may eliminate the corrsponding acess records,
    because reading function parameters is not considered a side-effect
    by mod-ref.  Is there easy way to detect such change?

 2) The propagation via jump functions (in applying inline decision)
    mixed bits and bytes in parameter offsets.
    Martin, it is not clear to me why the offset is in bits - there is no
    way to pass a pointer to non-byte aligned address and it seems that this
    only adds a risk of overflows.
    There seems to be overflow check missing in
    update_jump_functions_after_inlining, but it seems to me that these days
    this should be poly_int64.

 3) There was bug disabling late mod-ref during IPA, since the ipa flag was
    set incorrectly in the summary.

This made it bit hard to work out what happens in dealII and x264. They was
both hit primarily by 2) but if there was no 3) it would not trigger the
problem, so I did not get the idea to doube-check the jump function
handling.

1) seems to be surprisingly harmless because we have only one clone in
dealII that needs updating. It seems that sra and ipa-cp heuristics are
still way too conservative.

Current disambiguation stats for cc1plus are:

Alias oracle query stats:
  refs_may_alias_p: 63936508 disambiguations, 74133491 queries
  ref_maybe_used_by_call_p: 141601 disambiguations, 64838386 queries
  call_may_clobber_ref_p: 22977 disambiguations, 28758 queries
  nonoverlapping_component_refs_p: 0 disambiguations, 37176 queries
  nonoverlapping_refs_since_match_p: 19409 disambiguations, 55558 must overlaps, 75758 queries
  aliasing_component_refs_p: 54733 disambiguations, 755912 queries
  TBAA oracle: 22986556 disambiguations 55156567 queries
               16068160 are in alias set 0
               10559362 queries asked about the same object
               125 queries asked about the same alias set
               0 access volatile
               3912975 are dependent in the DAG
               1629389 are aritificially in conflict with void *

Modref stats:
  modref use: 11111 disambiguations, 39535 queries
  modref clobber: 1399913 disambiguations, 1719874 queries
  3500169 tbaa queries (2.035131 per modref query)
  621878 base compares (0.361583 per modref query)

PTA query stats:
  pt_solution_includes: 967033 disambiguations, 13594707 queries
  pt_solutions_intersect: 1032740 disambiguations, 13097793 queries

This is comparable to previous stats
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555314.html
those seems to have similar disambiguation stats but more querries.
This may be related to
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555812.html
that disables some TBAA disambiguation where gimple memory model requires so.
(indeed modref TBAA query count is down by 36%)

Bootstrapped/regtested x86_64-linux, comitted (in 4 commits for which I
apologize. I was intending to commit to branch)

2020-10-10  Jan Hubicka  <hubicka@ucw.cz>

	* ipa-modref-tree.h (struct modref_tree): Revert prevoius change.
	* ipa-modref.c (analyze_function): Dump original summary.
	(modref_read): Only set IPA if streaming summary (not optimization
	summary).
	(remap_arguments): New function.
	(modref_transform): New function.
	(compute_parm_map): Fix offset calculation.
	(ipa_merge_modref_summary_after_inlining): Do not merge stores when
	they can be ignored.

Comments

Martin Jambor Oct. 13, 2020, 1:31 p.m. UTC | #1 
Hi,

On Sun, Oct 11 2020, Jan Hubicka wrote:
> Hi,
> this patch fixes the range tracking in argument and re-enables it for clones
> (the bug that broke dealII and x264 benchmarks)
>
> It turned out that there was three problems
>  1) for SRA/ipa-cp clones we did not update summarries to represent new
>     signature.  This is now done in modref_transform.
>     I tested it in ipa-sra testcases and it seems to work fine, but Martin,
>     can you please take a look?
>
>     Param adjustment interface provides original indexes for new indexes of
>     parameters.  I need reverse information: for original index I need new
>     that I compute via array map and then do the rewrite.

Looks OK, I only wonder whether the functionality cannot be more
generally useful and so whether computation of the reverse map should
not be made part of ipa_param_adjustments interface.

>
>     Martin, if things passed by references are translated to stuff
>     passed by value, we may eliminate the corrsponding acess records,
>     because reading function parameters is not considered a side-effect
>     by mod-ref.  Is there easy way to detect such change?

Such new parameter will have IPA_PARAM_OP_SPLIT as the operation in its
ipa_adjusted_param and so ipa_param_adjustments::get_original_index will
already return -1 for it, if that is enough.  The way to detect that is
currently only the fact that a pointer argument is split (new IPA SRA no
longer splits a reference into sub-references).  If the old parameter is
no longer available we need to add a flag, I'm afraid.

>
>  2) The propagation via jump functions (in applying inline decision)
>     mixed bits and bytes in parameter offsets.
>     Martin, it is not clear to me why the offset is in bits - there is no
>     way to pass a pointer to non-byte aligned address and it seems that this
>     only adds a risk of overflows.

Yeah, when I started writing the first versions of the code I simply
used directly the types that get_ref_base_and_extent uses.  I hope it's
all in HOST_WIDE_INTs and so should not really overflow but it wastes
memory.  Converting all of this to bytes is on my TODO list (and new
IPA-SRA already does that in the IPA phase).

>     There seems to be overflow check missing in
>     update_jump_functions_after_inlining, but it seems to me that these days
>     this should be poly_int64.

Not sure about this.  When the conversion to poly_ints happened,
get_ref_base_and_extent_hwi was introduced for the purposes of IPA
stuff, so I think even the poly_int authors did think it was the right
thing to use here.  In any case, I'm still having hard time wrapping my
head around poly_ints, so I'd like to see a testcase before going down
this route.

Thanks,

Martin
diff mbox series

Patch

diff --git a/gcc/ipa-modref-tree.h b/gcc/ipa-modref-tree.h
index 8d7f2864793..b37280d18c7 100644
--- a/gcc/ipa-modref-tree.h
+++ b/gcc/ipa-modref-tree.h
@@ -496,8 +496,7 @@  struct GTY((user)) modref_tree
   /* Copy OTHER to THIS.  */
   void copy_from (modref_tree <T> *other)
   {
-    auto_vec <modref_parm_map, 32> parm_map;
-    merge (other, &parm_map);
+    merge (other, NULL);
   }
 
   /* Search BASE in tree; return NULL if failed.  */
diff --git a/gcc/ipa-modref.c b/gcc/ipa-modref.c
index c22c0d233f7..dd59e804c0f 100644
--- a/gcc/ipa-modref.c
+++ b/gcc/ipa-modref.c
@@ -757,7 +757,14 @@  analyze_function (function *f, bool ipa)
   if (!summaries)
     summaries = modref_summaries::create_ggc (symtab);
   else /* Remove existing summary if we are re-running the pass.  */
-    summaries->remove (cgraph_node::get (f->decl));
+    {
+      if (dump_file && summaries->get (cgraph_node::get (f->decl)))
+	{
+	  fprintf (dump_file, "Past summary:\n");
+	  summaries->get (cgraph_node::get (f->decl))->dump (dump_file);
+	}
+      summaries->remove (cgraph_node::get (f->decl));
+    }
 
   ((modref_summaries *)summaries)->ipa = ipa;
 
@@ -1290,7 +1297,7 @@  modref_read (void)
 
   if (!summaries)
     summaries = modref_summaries::create_ggc (symtab);
-  ((modref_summaries *)summaries)->ipa = true;
+  ((modref_summaries *)summaries)->ipa = !flag_ltrans;
 
   while ((file_data = file_data_vec[j++]))
     {
@@ -1309,6 +1316,81 @@  modref_read (void)
     }
 }
 
+/* Update parameter indexes in TT according to MAP.  */
+
+void
+remap_arguments (vec <int> *map, modref_records *tt)
+{
+  size_t i;
+  modref_base_node <alias_set_type> *base_node;
+  FOR_EACH_VEC_SAFE_ELT (tt->bases, i, base_node)
+    {
+      size_t j;
+      modref_ref_node <alias_set_type> *ref_node;
+      FOR_EACH_VEC_SAFE_ELT (base_node->refs, j, ref_node)
+	{
+	  size_t k;
+	  modref_access_node *access_node;
+	  FOR_EACH_VEC_SAFE_ELT (ref_node->accesses, k, access_node)
+	    if (access_node->parm_index > 0)
+	      {
+		if (access_node->parm_index < (int)map->length ())
+		  access_node->parm_index = (*map)[access_node->parm_index];
+		else
+		  access_node->parm_index = -1;
+	      }
+	}
+    }
+}
+
+/* If signature changed, update the summary.  */
+
+static unsigned int
+modref_transform (struct cgraph_node *node)
+{
+  if (!node->clone.param_adjustments || !summaries)
+    return 0;
+  modref_summary *r = summaries->get (node);
+  if (!r)
+    return 0;
+  if (dump_file)
+    {
+      fprintf (dump_file, "Updating summary for %s from:\n",
+	       node->dump_name ());
+      r->dump (dump_file);
+    }
+
+  size_t i, max = 0;
+  ipa_adjusted_param *p;
+
+  FOR_EACH_VEC_SAFE_ELT (node->clone.param_adjustments->m_adj_params, i, p)
+    {
+      int idx = node->clone.param_adjustments->get_original_index (i);
+      if (idx > (int)max)
+	max = idx;
+    }
+
+  auto_vec <int, 32> map;
+
+  map.reserve (max + 1);
+  for (i = 0; i <= max; i++)
+    map.quick_push (-1);
+  FOR_EACH_VEC_SAFE_ELT (node->clone.param_adjustments->m_adj_params, i, p)
+    {
+      int idx = node->clone.param_adjustments->get_original_index (i);
+      if (idx >= 0)
+	map[idx] = i;
+    }
+  remap_arguments (&map, r->loads);
+  remap_arguments (&map, r->stores);
+  if (dump_file)
+    {
+      fprintf (dump_file, "to:\n");
+      r->dump (dump_file);
+    }
+  return 0;
+}
+
 /* Definition of the modref IPA pass.  */
 const pass_data pass_data_ipa_modref =
 {
@@ -1335,7 +1417,7 @@  public:
 		      modref_read,     /* read_optimization_summary */
 		      NULL,            /* stmt_fixup */
 		      0,               /* function_transform_todo_flags_start */
-		      NULL,            /* function_transform */
+		      modref_transform,/* function_transform */
 		      NULL)            /* variable_transform */
   {}
 
@@ -1448,7 +1530,10 @@  compute_parm_map (cgraph_edge *callee_edge, vec<modref_parm_map> *parm_map)
 	    {
 	      (*parm_map)[i].parm_index = ipa_get_jf_ancestor_formal_id (jf);
 	      (*parm_map)[i].parm_offset_known = true;
-	      (*parm_map)[i].parm_offset = ipa_get_jf_ancestor_offset (jf);
+	      gcc_checking_assert
+		(!(ipa_get_jf_ancestor_offset (jf) & (BITS_PER_UNIT - 1)));
+	      (*parm_map)[i].parm_offset
+		 = ipa_get_jf_ancestor_offset (jf) >> LOG2_BITS_PER_UNIT;
  	    }
 	  else
 	    (*parm_map)[i].parm_index = -1;
@@ -1503,10 +1588,13 @@  ipa_merge_modref_summary_after_inlining (cgraph_edge *edge)
 
       compute_parm_map (edge, &parm_map);
 
-      if (to_info->loads)
-	to_info->loads->merge (callee_info->loads, &parm_map);
-      if (to_info->stores)
-	to_info->stores->merge (callee_info->stores, &parm_map);
+      if (!ignore_stores_p (edge->callee->decl, flags))
+	{
+	  if (to_info->loads)
+	    to_info->loads->merge (callee_info->loads, &parm_map);
+	  if (to_info->stores)
+	    to_info->stores->merge (callee_info->stores, &parm_map);
+	}
       if (to_info->loads_lto)
 	to_info->loads_lto->merge (callee_info->loads_lto, &parm_map);
       if (to_info->stores_lto)