[PATCH] Fix IPA-ICF wrong-code with GIMPLE_PREDICT (PR ipa/65765)

[PATCH] Fix IPA-ICF wrong-code with GIMPLE_PREDICT (PR ipa/65765)

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Hi!

When ICF encounteres a GIMPLE_PREDICT (as in the testcase) or GIMPLE_NOP,
it stops looking at further statements in the bb (assumes they must be the
same), which is of course wrong.

I'm attaching 3 versions of the patch, the first one I've
bootstrapped/regtested on x86_64-linux and i686-linux, the second one
will handle GIMPLE_PREDICT just as (fixed) GIMPLE_NOP handling, as Honza is
saying that at this point GIMPLE_PREDICT can be ignored (but we e.g. risk
when using the same function for something different that we'll not handle
GIMPLE_PREDICT right), the third one is the most minimal one (but I'm afraid
the lack of GIMPLE_EH_DISPATCH region checking might cause wrong-code).

Ok for trunk (which version) and 5.1 (which version)?

	Jakub

[-- Attachment #2: V563 --]
[-- Type: text/plain, Size: 2401 bytes --]

2015-04-15  Jakub Jelinek  <jakub@redhat.com>

	PR ipa/65765
	* ipa-icf-gimple.c (func_checker::compare_bb): For GIMPLE_NOP,
	use break instead of return true.  For GIMPLE_PREDICT, compare
	predictor and outcome, and break instead of return true if it is
	the same.  For GIMPLE_EH_DISPATCH, compare dispatch region.

	* g++.dg/ipa/pr65765.C: New test.

--- gcc/ipa-icf-gimple.c.jj	2015-04-12 21:29:24.000000000 +0200
+++ gcc/ipa-icf-gimple.c	2015-04-15 09:08:19.637659016 +0200
@@ -706,7 +706,11 @@ func_checker::compare_bb (sem_bb *bb1, s
 	    return return_different_stmts (s1, s2, "GIMPLE_SWITCH");
 	  break;
 	case GIMPLE_DEBUG:
+	  break;
 	case GIMPLE_EH_DISPATCH:
+	  if (gimple_eh_dispatch_region (as_a <geh_dispatch *> (s1))
+	      != gimple_eh_dispatch_region (as_a <geh_dispatch *> (s2)))
+	    return return_different_stmts (s1, s2, "GIMPLE_EH_DISPATCH");
 	  break;
 	case GIMPLE_RESX:
 	  if (!compare_gimple_resx (as_a <gresx *> (s1),
@@ -733,8 +737,12 @@ func_checker::compare_bb (sem_bb *bb1, s
 	    return return_different_stmts (s1, s2, "GIMPLE_ASM");
 	  break;
 	case GIMPLE_PREDICT:
+	  if (gimple_predict_predictor (s1) != gimple_predict_predictor (s2)
+	      || gimple_predict_outcome (s1) != gimple_predict_outcome (s2))
+	    return return_different_stmts (s1, s2, "GIMPLE_PREDICT");
+	  break;
 	case GIMPLE_NOP:
-	  return true;
+	  break;
 	default:
 	  return return_false_with_msg ("Unknown GIMPLE code reached");
 	}
--- gcc/testsuite/g++.dg/ipa/pr65765.C.jj	2015-04-15 09:14:03.329930193 +0200
+++ gcc/testsuite/g++.dg/ipa/pr65765.C	2015-04-15 09:13:41.000000000 +0200
@@ -0,0 +1,45 @@
+// PR ipa/65765
+// { dg-do run }
+// { dg-options "-O2" }
+
+int a, b, c, d, e;
+unsigned char h[] = { 1, 1 };
+
+__attribute__ ((cold)) int ModRM_Mode () { return a; }
+
+int
+ModRM_RM (int p1)
+{
+  return p1;
+}
+
+__attribute__ ((cold)) static bool ModRM_hasSIB (unsigned char p1)
+{
+  return ModRM_Mode () != 1 && ModRM_RM (p1);
+}
+
+__attribute__ ((cold)) static bool ModRM_hasRIP (unsigned char p1)
+{
+  return ModRM_Mode () && ModRM_RM (p1);
+}
+
+unsigned char *
+DisassembleHeapAccess (unsigned char *p1)
+{
+  b = *p1++;
+  if (ModRM_hasSIB (b))
+    c = *p1++;
+  int f = c, g = 0;
+  d = ModRM_hasRIP (g);
+  e = f == 0;
+  if (e)
+    p1 += sizeof 0;
+  return p1;
+}
+
+int
+main ()
+{
+  if (DisassembleHeapAccess (h) != h + 2)
+    __builtin_abort ();
+}

[-- Attachment #3: V563a --]
[-- Type: text/plain, Size: 2035 bytes --]

2015-04-15  Jakub Jelinek  <jakub@redhat.com>

	PR ipa/65765
	* ipa-icf-gimple.c (func_checker::compare_bb): For GIMPLE_NOP
	and GIMPLE_PREDICT use break instead of return true. For
	GIMPLE_EH_DISPATCH, compare dispatch region.

	* g++.dg/ipa/pr65765.C: New test.

--- gcc/ipa-icf-gimple.c.jj	2015-04-12 21:29:24.000000000 +0200
+++ gcc/ipa-icf-gimple.c	2015-04-15 09:08:19.637659016 +0200
@@ -706,7 +706,11 @@ func_checker::compare_bb (sem_bb *bb1, s
 	    return return_different_stmts (s1, s2, "GIMPLE_SWITCH");
 	  break;
 	case GIMPLE_DEBUG:
+	  break;
 	case GIMPLE_EH_DISPATCH:
+	  if (gimple_eh_dispatch_region (as_a <geh_dispatch *> (s1))
+	      != gimple_eh_dispatch_region (as_a <geh_dispatch *> (s2)))
+	    return return_different_stmts (s1, s2, "GIMPLE_EH_DISPATCH");
 	  break;
 	case GIMPLE_RESX:
 	  if (!compare_gimple_resx (as_a <gresx *> (s1),
@@ -734,7 +738,7 @@ func_checker::compare_bb (sem_bb *bb1, s
 	  break;
 	case GIMPLE_PREDICT:
 	case GIMPLE_NOP:
-	  return true;
+	  break;
 	default:
 	  return return_false_with_msg ("Unknown GIMPLE code reached");
 	}
--- gcc/testsuite/g++.dg/ipa/pr65765.C.jj	2015-04-15 09:14:03.329930193 +0200
+++ gcc/testsuite/g++.dg/ipa/pr65765.C	2015-04-15 09:13:41.000000000 +0200
@@ -0,0 +1,45 @@
+// PR ipa/65765
+// { dg-do run }
+// { dg-options "-O2" }
+
+int a, b, c, d, e;
+unsigned char h[] = { 1, 1 };
+
+__attribute__ ((cold)) int ModRM_Mode () { return a; }
+
+int
+ModRM_RM (int p1)
+{
+  return p1;
+}
+
+__attribute__ ((cold)) static bool ModRM_hasSIB (unsigned char p1)
+{
+  return ModRM_Mode () != 1 && ModRM_RM (p1);
+}
+
+__attribute__ ((cold)) static bool ModRM_hasRIP (unsigned char p1)
+{
+  return ModRM_Mode () && ModRM_RM (p1);
+}
+
+unsigned char *
+DisassembleHeapAccess (unsigned char *p1)
+{
+  b = *p1++;
+  if (ModRM_hasSIB (b))
+    c = *p1++;
+  int f = c, g = 0;
+  d = ModRM_hasRIP (g);
+  e = f == 0;
+  if (e)
+    p1 += sizeof 0;
+  return p1;
+}
+
+int
+main ()
+{
+  if (DisassembleHeapAccess (h) != h + 2)
+    __builtin_abort ();
+}

[-- Attachment #4: V563b --]
[-- Type: text/plain, Size: 1510 bytes --]

2015-04-15  Jakub Jelinek  <jakub@redhat.com>

	PR ipa/65765
	* ipa-icf-gimple.c (func_checker::compare_bb): For GIMPLE_NOP
	and GIMPLE_PREDICT use break instead of return true.

	* g++.dg/ipa/pr65765.C: New test.

--- gcc/ipa-icf-gimple.c.jj	2015-04-12 21:29:24.000000000 +0200
+++ gcc/ipa-icf-gimple.c	2015-04-15 09:08:19.637659016 +0200
@@ -734,7 +734,7 @@ func_checker::compare_bb (sem_bb *bb1, s
 	  break;
 	case GIMPLE_PREDICT:
 	case GIMPLE_NOP:
-	  return true;
+	  break;
 	default:
 	  return return_false_with_msg ("Unknown GIMPLE code reached");
 	}
--- gcc/testsuite/g++.dg/ipa/pr65765.C.jj	2015-04-15 09:14:03.329930193 +0200
+++ gcc/testsuite/g++.dg/ipa/pr65765.C	2015-04-15 09:13:41.000000000 +0200
@@ -0,0 +1,45 @@
+// PR ipa/65765
+// { dg-do run }
+// { dg-options "-O2" }
+
+int a, b, c, d, e;
+unsigned char h[] = { 1, 1 };
+
+__attribute__ ((cold)) int ModRM_Mode () { return a; }
+
+int
+ModRM_RM (int p1)
+{
+  return p1;
+}
+
+__attribute__ ((cold)) static bool ModRM_hasSIB (unsigned char p1)
+{
+  return ModRM_Mode () != 1 && ModRM_RM (p1);
+}
+
+__attribute__ ((cold)) static bool ModRM_hasRIP (unsigned char p1)
+{
+  return ModRM_Mode () && ModRM_RM (p1);
+}
+
+unsigned char *
+DisassembleHeapAccess (unsigned char *p1)
+{
+  b = *p1++;
+  if (ModRM_hasSIB (b))
+    c = *p1++;
+  int f = c, g = 0;
+  d = ModRM_hasRIP (g);
+  e = f == 0;
+  if (e)
+    p1 += sizeof 0;
+  return p1;
+}
+
+int
+main ()
+{
+  if (DisassembleHeapAccess (h) != h + 2)
+    __builtin_abort ();
+}

Re: [PATCH] Fix IPA-ICF wrong-code with GIMPLE_PREDICT (PR ipa/65765)

[Richard Biener]

On Wed, 15 Apr 2015, Jakub Jelinek wrote:

> Hi!
> 
> When ICF encounteres a GIMPLE_PREDICT (as in the testcase) or GIMPLE_NOP,
> it stops looking at further statements in the bb (assumes they must be the
> same), which is of course wrong.
> 
> I'm attaching 3 versions of the patch, the first one I've
> bootstrapped/regtested on x86_64-linux and i686-linux, the second one
> will handle GIMPLE_PREDICT just as (fixed) GIMPLE_NOP handling, as Honza is
> saying that at this point GIMPLE_PREDICT can be ignored (but we e.g. risk
> when using the same function for something different that we'll not handle
> GIMPLE_PREDICT right), the third one is the most minimal one (but I'm afraid
> the lack of GIMPLE_EH_DISPATCH region checking might cause wrong-code).
> 
> Ok for trunk (which version) and 5.1 (which version)?

Ok for trunk and 5.1, version without GIMPLE_PREDICT handling.  Let's
iterate on that separately.

Thanks,
Richard.

Re: [PATCH] Fix IPA-ICF wrong-code with GIMPLE_PREDICT (PR ipa/65765)

[Jan Hubicka]

> Hi!
> 
> When ICF encounteres a GIMPLE_PREDICT (as in the testcase) or GIMPLE_NOP,
> it stops looking at further statements in the bb (assumes they must be the
> same), which is of course wrong.
> 
> I'm attaching 3 versions of the patch, the first one I've
> bootstrapped/regtested on x86_64-linux and i686-linux, the second one
> will handle GIMPLE_PREDICT just as (fixed) GIMPLE_NOP handling, as Honza is
> saying that at this point GIMPLE_PREDICT can be ignored (but we e.g. risk
> when using the same function for something different that we'll not handle
> GIMPLE_PREDICT right), the third one is the most minimal one (but I'm afraid

Sorry for somewhat terse answer, I was online just briefly and could not check
the code.  Matching GIMPLE_PREDICT won't help you here.  The predicts are
consumed by pass_profile during early optimizations and converted into
edge probabilities. They get removed, but only in pass_strip_predict_hints
that is run as first late optimization.

During IPA optimization they just sit there and are fully ignored. The reason
why pass_strip_predict_hints is that we want to keep the hints during early
inlining (so if function is inlined, the hints are applied to the inlined body,
too).

With LTO world, it proably would make moresnese to schedule predict hint removal
just before we compute IPA summaries, so we do not need to stream them in/out
for no reason.

Richard alredy approved the change for GCC 5 that is IMO fully sufficient.
We may want to keep your GIMPLE_PREDICT code if we start doing ipa-icf-gimple.c 
driven tail merging during early optimizations (which is a plan).

Honza