Patchwork Fix bt[lq] related miscompilation (PR target/48774)

login
register
mail settings
Submitter Jakub Jelinek
Date May 3, 2011, 12:39 p.m.
Message ID <20110503123959.GY17079@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/93782/
State New
Headers show

Comments

Jakub Jelinek - May 3, 2011, 12:39 p.m.
On Tue, May 03, 2011 at 10:26:50AM +0200, Uros Bizjak wrote:
> On Mon, May 2, 2011 at 10:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> IMO, this problem arises due to wrong fix for PR target/37184 [1] that
> added CCA, CCC, CCO, CCS mode bypasses to ix86_match_ccmode.

Yeah, ix86_match_ccmode was the first place I was looking at and
it surprised me it allowed all those modes for CCNOmode req_mode.
Thought there was a reason why it is done that way, but if there is not,
even better.  CC{A,O,S,Z}mode seem to be apparently only used for SSE
intrinsics and CCCmode just for bt and adc/sbb etc.

> Jakub, can you please check the patch, I'm not able to test it
> properly until tonight.

Yeah, following patch passed bootstrap/regtest on x86_64-linux and
i686-linux.  Ok for trunk/4.6 (and perhaps later on 4.5/4.4)?

2011-05-03  Uros Bizjak  <ubizjak@gmail.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR target/48774
	* config/i386/i386.c (ix86_match_ccmode): For CC{A,C,O,S}mode
	only succeed if req_mode is the same as set_mode.

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



	Jakub
Uros Bizjak - May 3, 2011, 12:48 p.m.
On Tue, May 3, 2011 at 2:39 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, May 03, 2011 at 10:26:50AM +0200, Uros Bizjak wrote:
>> On Mon, May 2, 2011 at 10:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> IMO, this problem arises due to wrong fix for PR target/37184 [1] that
>> added CCA, CCC, CCO, CCS mode bypasses to ix86_match_ccmode.
>
> Yeah, ix86_match_ccmode was the first place I was looking at and
> it surprised me it allowed all those modes for CCNOmode req_mode.
> Thought there was a reason why it is done that way, but if there is not,
> even better.  CC{A,O,S,Z}mode seem to be apparently only used for SSE
> intrinsics and CCCmode just for bt and adc/sbb etc.

Heh, there was no reason that these modes were added that way... just
to bypass gcc_unreachable (), see referred PR.

>> Jakub, can you please check the patch, I'm not able to test it
>> properly until tonight.
>
> Yeah, following patch passed bootstrap/regtest on x86_64-linux and
> i686-linux.  Ok for trunk/4.6 (and perhaps later on 4.5/4.4)?
>
> 2011-05-03  Uros Bizjak  <ubizjak@gmail.com>
>            Jakub Jelinek  <jakub@redhat.com>
>
>        PR target/48774
>        * config/i386/i386.c (ix86_match_ccmode): For CC{A,C,O,S}mode
>        only succeed if req_mode is the same as set_mode.
>
>        * gcc.dg/pr48774.c: New test.

This is OK everywhere, it was a thinko after all.

Thanks,
Uros.

Patch

--- gcc/config/i386/i386.c.jj	2011-05-03 12:11:23.000000000 +0200
+++ gcc/config/i386/i386.c	2011-05-03 12:24:05.320670624 +0200
@@ -17299,11 +17299,15 @@  ix86_match_ccmode (rtx insn, enum machin
       if (req_mode == CCZmode)
 	return false;
       /* FALLTHRU */
+    case CCZmode:
+      break;
+
     case CCAmode:
     case CCCmode:
     case CCOmode:
     case CCSmode:
-    case CCZmode:
+      if (set_mode != req_mode)
+	return false;
       break;
 
     default:
--- gcc/testsuite/gcc.dg/pr48774.c.jj	2011-05-02 19:12:32.000000000 +0200
+++ gcc/testsuite/gcc.dg/pr48774.c	2011-05-02 11:03:55.000000000 +0200
@@ -0,0 +1,38 @@ 
+/* PR target/48774 */
+/* { dg-do run } */
+/* { dg-options "-O2 -funroll-loops" } */
+
+extern void abort (void);
+unsigned long int s[24]
+  = { 12, ~1, 12, ~2, 12, ~4, 12, ~8, 12, ~16, 12, ~32,
+      12, ~64, 12, ~128, 12, ~256, 12, ~512, 12, ~1024, 12, ~2048 };
+struct { int n; unsigned long *e[12]; } g
+  = { 12, { &s[0], &s[2], &s[4], &s[6], &s[8], &s[10], &s[12], &s[14],
+	    &s[16], &s[18], &s[20], &s[22] } };
+int c[12];
+
+__attribute__((noinline, noclone)) void
+foo (void)
+{
+  int i, j;
+  for (i = 0; i < g.n; i++)
+    for (j = 0; j < g.n; j++)
+      {
+	if (i == j && j < g.e[0][0] && (g.e[i][1] & (1UL << j)))
+	  abort ();
+	if (j < g.e[0][0] && (g.e[i][1] & (1UL << j)))
+	  c[i]++;
+      }
+}
+
+int
+main ()
+{
+  int i;
+  asm volatile ("" : "+m" (s), "+m" (g), "+m" (c));
+  foo ();
+  for (i = 0; i < 12; i++)
+    if (c[i] != 11)
+      abort ();
+  return 0;
+}