[PATCH 3/5] IPA ICF pass

Jan Hubicka hubicka@ucw.cz
Fri Sep 26 20:47:00 GMT 2014


Hi,
this is on ipa-icf-gimple.c

@@ -2827,11 +2829,19 @@ cgraph_node::verify_node (void)
 			    {
 			      if (verify_edge_corresponds_to_fndecl (e, decl))
 				{
-				  error ("edge points to wrong declaration:");
-				  debug_tree (e->callee->decl);
-				  fprintf (stderr," Instead of:");
-				  debug_tree (decl);
-				  error_found = true;
+				  /* The edge can be redirected in WPA by IPA ICF.
+				     Following check really ensures that it's
+				     not the case.  */
+
+				  cgraph_node *current_node = cgraph_node::get (decl);
+				  if (!current_node || !current_node->icf_merged)

I would move this into verify_edge_corresponds_to_fndecl.

diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
new file mode 100644
index 0000000..7031eaa
--- /dev/null
+++ b/gcc/ipa-icf-gimple.c
@@ -0,0 +1,384 @@
+/* Interprocedural Identical Code Folding pass
+   Copyright (C) 2014 Free Software Foundation, Inc.
+
+   Contributed by Jan Hubicka <hubicka@ucw.cz> and Martin Liska <mliska@suse.cz>
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */

Please add toplevel comment about what the code does and how to use it.

+namespace ipa_icf {
+
+/* Basic block equivalence comparison function that returns true if
+   basic blocks BB1 and BB2 (from functions FUNC1 and FUNC2) correspond.  */
... to each other?
I would add short comment that as comparsion goes you build voclabulary
of equivalences of variables/ssanames etc.
So people reading the code do not get lost at very beggining.

+
+bool
+func_checker::compare_bb (sem_bb *bb1, sem_bb *bb2)
+{
+  unsigned i;
+  gimple_stmt_iterator gsi1, gsi2;
+  gimple s1, s2;
+
+  if (bb1->nondbg_stmt_count != bb2->nondbg_stmt_count
+      || bb1->edge_count != bb2->edge_count)
+    return RETURN_FALSE ();

The UPPERCASE looks ugly.  I see that RETURN_FALSE is a warpper for return_false_with_msg
that outputs line and file information.

I would make it lowercase even if it is macro. You may consider using
CXX_MEM_STAT_INFO style default argument to avoid function macro completely.
Probably not big win given that it won't save you from preprocesor mess.
+
+  gsi1 = gsi_start_bb (bb1->bb);
+  gsi2 = gsi_start_bb (bb2->bb);
+
+  for (i = 0; i < bb1->nondbg_stmt_count; i++)
+    {
+      if (is_gimple_debug (gsi_stmt (gsi1)))
+	gsi_next_nondebug (&gsi1);
+
+      if (is_gimple_debug (gsi_stmt (gsi2)))
+	gsi_next_nondebug (&gsi2);
+
+      s1 = gsi_stmt (gsi1);
+      s2 = gsi_stmt (gsi2);
+
+      if (gimple_code (s1) != gimple_code (s2))
+	return RETURN_FALSE_WITH_MSG ("gimple codes are different");

I think you need to compare EH here.  Consider case where one unit
is compiled with -fno-exception and thus all EH regions are removed,
while other function has EH regions in it.  Those are not equivalent.

EH region is obtained by lookup_stmt_eh and then you need to comapre
them for match as you do with gimple_resx_regoin.

+  t1 = gimple_call_fndecl (s1);
+  t2 = gimple_call_fndecl (s2);
+
+  /* Function pointer variables are not supported yet.  */

They seems to be, compare_operand seems just right.

+
+/* Verifies for given GIMPLEs S1 and S2 that
+   label statements are semantically equivalent.  */
+
+bool
+func_checker::compare_gimple_label (gimple g1, gimple g2)
+{
+  if (m_ignore_labels)
+    return true;
+
+  tree t1 = gimple_label_label (g1);
+  tree t2 = gimple_label_label (g2);
+
+  return compare_tree_ssa_label (t1, t2);
+}

I would expect the main BB loop to record BB in which label belongs to
and the BB assciatio neing checked here.
Otherwise I do not see how switch statements are compared to not have
different permutations of targets. Also note that one BB may have
multiple labels in them and they are equivalent.

Also I would punt on occurence of FORCED_LABEL. Those are tricky as they
may be passed around and compared for address and no one really defines
what should happen.  Better to avoid those.

+
+/* Verifies for given GIMPLEs S1 and S2 that
+   switch statements are semantically equivalent.  */
+
+bool
+func_checker::compare_gimple_switch (gimple g1, gimple g2)
+{
+  unsigned lsize1, lsize2, i;
+  tree t1, t2, low1, low2, high1, high2;
+
+  lsize1 = gimple_switch_num_labels (g1);
+  lsize2 = gimple_switch_num_labels (g2);
+
+  if (lsize1 != lsize2)
+    return false;
+
+  t1 = gimple_switch_index (g1);
+  t2 = gimple_switch_index (g2);
+
+  if (TREE_CODE (t1) != SSA_NAME || TREE_CODE(t2) != SSA_NAME)
+    return false;

Why non-SSA_NAMES are excluded? I see that constants should be optimized out,
but I do not see a need for specific test here.
+
+  if (!compare_operand (t1, t2))
+    return false;
+
+  for (i = 0; i < lsize1; i++)
+    {
+      low1 = CASE_LOW (gimple_switch_label (g1, i));
+      low2 = CASE_LOW (gimple_switch_label (g2, i));
+
+      if ((low1 != NULL) != (low2 != NULL)
+	  || (low1 && low2 && TREE_INT_CST_LOW (low1) != TREE_INT_CST_LOW (low2)))
+	return false;

You want to compare integers for equivality.  Just use tree_int_cst_equal.
+
+      high1 = CASE_HIGH (gimple_switch_label (g1, i));
+      high2 = CASE_HIGH (gimple_switch_label (g2, i));
+
+      if ((high1 != NULL) != (high2 != NULL)
+	  || (high1 && high2
+	      && TREE_INT_CST_LOW (high1) != TREE_INT_CST_LOW (high2)))
+	return false;

Same here.
+    }
+
+  return true;
+}
+
+/* Verifies for given GIMPLEs S1 and S2 that
+   return statements are semantically equivalent.  */
+
+bool
+func_checker::compare_gimple_return (gimple g1, gimple g2)
+{
+  tree t1, t2;
+
+  t1 = gimple_return_retval (g1);
+  t2 = gimple_return_retval (g2);
+
+  /* Void return type.  */
+  if (t1 == NULL && t2 == NULL)
+    return true;
+  else
+    return compare_operand (t1, t2);

Do you check somewhere DECL_BY_REFERENCE (DECL_RESULT (struct function))? Those needs to match, too.
Perhaps it is always the case that SSA form differs, but I would test it.
+}
+
+/* Verifies for given GIMPLEs S1 and S2 that
+   goto statements are semantically equivalent.  */
+
+bool
+func_checker::compare_gimple_goto (gimple g1, gimple g2)
+{
+  tree dest1, dest2;
+
+  dest1 = gimple_goto_dest (g1);
+  dest2 = gimple_goto_dest (g2);
+
+  if (TREE_CODE (dest1) != TREE_CODE (dest2) || TREE_CODE (dest1) != SSA_NAME)
+    return false;
+
+  return compare_operand (dest1, dest2);

You probably need to care only about indirect gotos, the direct ones are checked by
CFG compare.  So is the condtional jump.
+
+/* Verifies for given GIMPLEs S1 and S2 that ASM statements are equivalent.
+   For the beginning, the pass only supports equality for
+   '__asm__ __volatile__ ("", "", "", "memory")'.  */
+
+bool
+func_checker::compare_gimple_asm (gimple g1, gimple g2)
+{
+  if (gimple_asm_volatile_p (g1) != gimple_asm_volatile_p (g2))
+    return false;
+
+  if (gimple_asm_ninputs (g1) || gimple_asm_ninputs (g2))
+    return false;
+
+  if (gimple_asm_noutputs (g1) || gimple_asm_noutputs (g2))
+    return false;
+
+  if (gimple_asm_nlabels (g1) || gimple_asm_nlabels (g2))
+    return false;
+
+  if (gimple_asm_nclobbers (g1) != gimple_asm_nclobbers (g2))
+    return false;
+
+  for (unsigned i = 0; i < gimple_asm_nclobbers (g1); i++)
+    {
+      tree clobber1 = TREE_VALUE (gimple_asm_clobber_op (g1, i));
+      tree clobber2 = TREE_VALUE (gimple_asm_clobber_op (g2, i));
+
+      if (!operand_equal_p (clobber1, clobber2, OEP_ONLY_CONST))
+	return false;
+    }
+

Even asm statements with no inputs or outputs can differ by the actual
asm statement. Compare it too.

Comparing inputs/outputs/labels should be very easy to do.

Compare all gimple_asm_n* for equivalency.
At the end walk operands and watch the case they are TREE_LIST.
THen compare TREE_VALUE (op) of the list for operand_equal_p
and TREE_VALUE (TREE_PURPOSE (op)) for equivalency
(those are the constraints)

If they are not (clobbers are not, those are just strings), operand_equal_p
should do.

+  return true;
+}
+
+} // ipa_icf namespace

Otherwise I think ipa-gimple-icf is quite fine now.
Please send updated version and I think it can go to mainline before the actual ipa-icf.



More information about the Gcc-patches mailing list