icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907]

Message ID Zc29VHpxUFCDu2Yq@tucnak
State Unresolved
Headers
Series icf: Reset SSA_NAME_{PTR,RANGE}_INFO in successfully merged functions [PR113907] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Feb. 15, 2024, 7:29 a.m. UTC
  Hi!

AFAIK we have no code in LTO streaming to stream out or in
SSA_NAME_{RANGE,PTR}_INFO, so LTO effectively throws it all away
and let vrp1 and alias analysis after IPA recompute that.  There is
just one spot, for IPA VRP and IPA bit CCP we save/restore ranges
and set SSA_NAME_{PTR,RANGE}_INFO e.g. on parameters depending on what
we saved and propagated, but that is after streaming in bodies for the
post IPA optimizations.

Now, without LTO SSA_NAME_{RANGE,PTR}_INFO is already computed from
earlier in many cases (er.g. evrp and early alias analysis but other spots
too), but IPA ICF is ignoring the ranges and points-to details when
comparing the bodies.  I think ignoring that is just fine, that is
effectively what we do for LTO where we throw that information away
before the analysis, and not ignoring it could lead to fewer ICF merging
possibilities.

So, the following patch instead verifies that for LTO SSA_NAME_{PTR,RANGE}_INFO
just isn't there on SSA_NAMEs in functions into which other functions have
been ICFed, and for non-LTO throws that information away (which matches the
LTO behavior).

Another possibility would be to remember the SSA_NAME <-> SSA_NAME mapping
vector (just one of the 2) on successful sem_function::equals on the
sem_function which is not the chosen leader (e.g. how SSA_NAMEs in the
leader map to SSA_NAMEs in the other function) and use that vector
to union the ranges in sem_function::merge.  I can implement that for
comparison, but wanted to post this first if there is an agreement on
doing that or if Honza thinks we should take SSA_NAME_{RANGE,PTR}_INFO
into account.  I think we can compare SSA_NAME_RANGE_INFO, but have
no idea how to try to compare points to info.  And I think it will result
in less effective ICF for non-LTO vs. LTO unnecessarily.

Bootstrapped/regtested on x86_64-linux and i686-linux.

2024-02-15  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/113907
	* ipa-icf.cc (sem_item_optimizer::merge_classes): Reset
	SSA_NAME_RANGE_INFO and SSA_NAME_PTR_INFO on successfully ICF merged
	functions.

	* gcc.dg/pr113907.c: New test.


	Jakub
  

Patch

--- gcc/ipa-icf.cc.jj	2024-02-14 14:26:11.101933914 +0100
+++ gcc/ipa-icf.cc	2024-02-14 16:49:35.141518117 +0100
@@ -3396,6 +3397,7 @@  sem_item_optimizer::merge_classes (unsig
 	  continue;
 
 	sem_item *source = c->members[0];
+	bool this_merged_p = false;
 
 	if (DECL_NAME (source->decl)
 	    && MAIN_NAME_P (DECL_NAME (source->decl)))
@@ -3443,7 +3445,7 @@  sem_item_optimizer::merge_classes (unsig
 	    if (dbg_cnt (merged_ipa_icf))
 	      {
 		bool merged = source->merge (alias);
-		merged_p |= merged;
+		this_merged_p |= merged;
 
 		if (merged && alias->type == VAR)
 		  {
@@ -3452,6 +3454,35 @@  sem_item_optimizer::merge_classes (unsig
 		  }
 	      }
 	  }
+
+	merged_p |= this_merged_p;
+	if (this_merged_p
+	    && source->type == FUNC
+	    && (!flag_wpa || flag_checking))
+	  {
+	    unsigned i;
+	    tree name;
+	    FOR_EACH_SSA_NAME (i, name, DECL_STRUCT_FUNCTION (source->decl))
+	      {
+		/* We need to either merge or reset SSA_NAME_*_INFO.
+		   For merging we don't preserve the mapping between
+		   original and alias SSA_NAMEs from successful equals
+		   calls.  */
+		if (POINTER_TYPE_P (TREE_TYPE (name)))
+		  {
+		    if (SSA_NAME_PTR_INFO (name))
+		      {
+			gcc_checking_assert (!flag_wpa);
+			SSA_NAME_PTR_INFO (name) = NULL;
+		      }
+		  }
+		else if (SSA_NAME_RANGE_INFO (name))
+		  {
+		    gcc_checking_assert (!flag_wpa);
+		    SSA_NAME_RANGE_INFO (name) = NULL;
+		  }
+	      }
+	  }
       }
 
   if (!m_merged_variables.is_empty ())
--- gcc/testsuite/gcc.dg/pr113907.c.jj	2024-02-14 16:13:48.486555159 +0100
+++ gcc/testsuite/gcc.dg/pr113907.c	2024-02-14 16:13:29.198825045 +0100
@@ -0,0 +1,49 @@ 
+/* PR middle-end/113907 */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+/* { dg-additional-options "-minline-all-stringops" { target i?86-*-* x86_64-*-* } } */
+
+static inline int
+foo (int len, void *indata, void *outdata)
+{
+  if (len < 0 || (len & 7) != 0)
+    return 0;
+  if (len != 0 && indata != outdata)
+    __builtin_memcpy (outdata, indata, len);
+  return len;
+}
+
+static inline int
+bar (int len, void *indata, void *outdata)
+{
+  if (len < 0 || (len & 1) != 0)
+    return 0;
+  if (len != 0 && indata != outdata)
+    __builtin_memcpy (outdata, indata, len);
+  return len;
+}
+
+int (*volatile p1) (int, void *, void *) = foo;
+int (*volatile p2) (int, void *, void *) = bar;
+
+__attribute__((noipa)) int
+baz (int len, void *indata, void *outdata)
+{
+  if ((len & 6) != 0)
+    bar (len, indata, outdata);
+  else
+    foo (len, indata, outdata);
+}
+
+struct S { char buf[64]; } s __attribute__((aligned (8)));
+
+int
+main ()
+{
+  for (int i = 0; i < 64; ++i)
+    s.buf[i] = ' ' + i;
+  p2 (2, s.buf, s.buf + 33);
+  for (int i = 0; i < 64; ++i)
+    if (s.buf[i] != ' ' + ((i >= 33 && i < 35) ? i - 33 : i))
+      __builtin_abort ();
+}