[PATCH] x86: Revert patches to fix PR target/88794

[PATCH] x86: Revert patches to fix PR target/88794

[Wei Xiao]

Hi,

It turns out that the Intel 64 and IA-32 Architectures Software Developer
Manuals (SDM) description about the fixupimm intrinsic is incorrect. So we need
to revert 3 patches related to it: r265827, r266026 and r267160.
Sorry for the inconvenience.

Is it ok?

Wei

Re: [PATCH] x86: Revert patches to fix PR target/88794

[Uros Bizjak]

On Tue, Jan 15, 2019 at 3:40 PM Wei Xiao <wei.william.xiao@gmail.com> wrote:
>
> Hi,
>
> It turns out that the Intel 64 and IA-32 Architectures Software Developer
> Manuals (SDM) description about the fixupimm intrinsic is incorrect. So we need
> to revert 3 patches related to it: r265827, r266026 and r267160.
> Sorry for the inconvenience.
>
> Is it ok?

Yes, but please test the compiler after the revert. Please also create
a runtime testcase out of the testcase in the PR.

Thanks,
Uros.

> Wei

Re: [PATCH] x86: Revert patches to fix PR target/88794

[Jakub Jelinek]

On Tue, Jan 15, 2019 at 04:14:06PM +0100, Uros Bizjak wrote:
> On Tue, Jan 15, 2019 at 3:40 PM Wei Xiao <wei.william.xiao@gmail.com> wrote:
> >
> > Hi,
> >
> > It turns out that the Intel 64 and IA-32 Architectures Software Developer
> > Manuals (SDM) description about the fixupimm intrinsic is incorrect. So we need
> > to revert 3 patches related to it: r265827, r266026 and r267160.
> > Sorry for the inconvenience.
> >
> > Is it ok?
> 
> Yes, but please test the compiler after the revert. Please also create
> a runtime testcase out of the testcase in the PR.

For r267160, I'd expect you want to revert just the config/i386/ part and
keep the testcases, they should work even with the changes reverted, right?

	Jakub

Re: [PATCH] x86: Revert patches to fix PR target/88794

[Wei Xiao]

> > Yes, but please test the compiler after the revert. Please also create
> > a runtime testcase out of the testcase in the PR.
Yes, we have tested it but current runtime testcase can't cover the corner
case to expose the incorrectness of SDM. We will add some after the revert.

> For r267160, I'd expect you want to revert just the config/i386/ part and
> keep the testcases, they should work even with the changes reverted, right?
>
The testcase part also need to be reverted since we have changed them
according to the incorrect intrinsic list in SDM.

Jakub Jelinek <jakub@redhat.com> 于2019年1月15日周二 下午11:20写道：
>
> On Tue, Jan 15, 2019 at 04:14:06PM +0100, Uros Bizjak wrote:
> > On Tue, Jan 15, 2019 at 3:40 PM Wei Xiao <wei.william.xiao@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > It turns out that the Intel 64 and IA-32 Architectures Software Developer
> > > Manuals (SDM) description about the fixupimm intrinsic is incorrect. So we need
> > > to revert 3 patches related to it: r265827, r266026 and r267160.
> > > Sorry for the inconvenience.
> > >
> > > Is it ok?
> >
> > Yes, but please test the compiler after the revert. Please also create
> > a runtime testcase out of the testcase in the PR.
>
> For r267160, I'd expect you want to revert just the config/i386/ part and
> keep the testcases, they should work even with the changes reverted, right?
>
>         Jakub

Re: [PATCH] x86: Revert patches to fix PR target/88794

[Wei Xiao]

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

The original runtime testcases are incorrect and I have fixed them as attached.
Is it ok to do the revert and fix the testcases for trunk?

Wei

2019-01-16  Wei Xiao  <wei3.xiao@intel.com>

        * gcc.target/i386/avx512f-vfixupimmpd-2.c: Fix the test cases for
        VFIXUPIMM* intrinsics.
        * gcc.target/i386/avx512f-vfixupimmps-2.c: Ditto.
        * gcc.target/i386/avx512f-vfixupimmsd-2.c: Ditto.
        * gcc.target/i386/avx512f-vfixupimmss-2.c: Ditto.

[-- Attachment #2: fixupimm_testcases.diff --]
[-- Type: application/octet-stream, Size: 5207 bytes --]

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index f032b48..20ed218 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2019-01-16  Wei Xiao  <wei3.xiao@intel.com>
+
+	* gcc.target/i386/avx512f-vfixupimmpd-2.c: Fix the test cases for
+	VFIXUPIMM* intrinsics.
+	* gcc.target/i386/avx512f-vfixupimmps-2.c: Ditto.
+	* gcc.target/i386/avx512f-vfixupimmsd-2.c: Ditto.
+	* gcc.target/i386/avx512f-vfixupimmss-2.c: Ditto.
+
 2019-01-15  Nikhil Benesch  <nikhil.benesch@gmail.com>
 
         * gcc.misc-tests/godump-1.c: Add test case for typedef before
diff --git a/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmpd-2.c b/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmpd-2.c
index 7ce9806..98b5ed1 100644
--- a/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmpd-2.c
+++ b/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmpd-2.c
@@ -14,12 +14,12 @@
 
 
 static void
-CALC (double *r, double src, long long tbl)
+CALC (double *r, double dest, double src, long long tbl)
 {
   switch (tbl & 0xf)
     {
     case 0:
-      *r = src;
+      *r = dest;
       break;
     case 1:
       *r = src;
@@ -81,7 +81,7 @@ TEST (void)
 
 
   float vals[2] = { -10, 10 };
-  int controls[8] = {0x11111111, 0x77777777, 0x77777777, 0x88888888,
+  int controls[8] = {0, 0x11111111, 0x77777777, 0x88888888,
     0x99999999, 0xaaaaaaaa, 0xbbbbbbbb, 0xcccccccc};
 
   MASK_TYPE mask = MASK_VALUE;
@@ -96,7 +96,7 @@ TEST (void)
 	  res2.a[j] = DEFAULT_VALUE;
 	  res3.a[j] = DEFAULT_VALUE;
 
-	  CALC (&res_ref[j], s1.a[j], s2.a[j]);
+	  CALC (&res_ref[j], res1.a[j], s1.a[j], s2.a[j]);
 	}
 
       res1.x = INTRINSIC (_fixupimm_pd) (res1.x, s1.x, s2.x, 0);
diff --git a/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmps-2.c b/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmps-2.c
index ffd899e..e5a917f 100644
--- a/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmps-2.c
+++ b/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmps-2.c
@@ -13,12 +13,12 @@
 #include "float.h"
 
 static void
-CALC (float *r, float src, int tbl)
+CALC (float *r, float dest, float src, int tbl)
 {
   switch (tbl & 0xf)
     {
     case 0:
-      *r = src;
+      *r = dest;
       break;
     case 1:
       *r = src;
@@ -81,8 +81,8 @@ TEST (void)
 
 
   float vals[2] = { -10, 10 };
-  int controls[16] = { 0x11111111,
-    0x77777777, 0x88888888, 0x99999999,
+  int controls[16] = { 0,
+    0x11111111, 0x88888888, 0x99999999,
     0xaaaaaaaa, 0xbbbbbbbb, 0xcccccccc,
     0x77777777, 0x88888888, 0x99999999,
     0xaaaaaaaa, 0xbbbbbbbb, 0xcccccccc,
@@ -101,7 +101,7 @@ TEST (void)
 	  res2.a[j] = DEFAULT_VALUE;
 	  res3.a[j] = DEFAULT_VALUE;
 
-	  CALC (&res_ref[j], s1.a[j], s2.a[j]);
+	  CALC (&res_ref[j], res1.a[j], s1.a[j], s2.a[j]);
 	}
 
       res1.x = INTRINSIC (_fixupimm_ps) (res1.x, s1.x, s2.x, 0);
diff --git a/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmsd-2.c b/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmsd-2.c
index 62c8d4e..d3cd28c 100644
--- a/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmsd-2.c
+++ b/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmsd-2.c
@@ -10,12 +10,12 @@
 #include "avx512f-mask-type.h"
 
 void
-compute_fixupimmpd (double *r, double src, long long tbl)
+compute_fixupimmpd (double *r, double dest, double src, long long tbl)
 {
   switch (tbl & 0xf)
     {
     case 0:
-      *r = src;
+      *r = dest;
       break;
     case 1:
       *r = src;
@@ -76,7 +76,7 @@ avx512f_test (void)
   int i, j;
 
   float vals[2] = { -10, 10 };
-  int controls[10] = { 0x11111111,
+  int controls[10] = { 0,
     0x77777777, 0x88888888, 0x99999999,
     0xaaaaaaaa, 0xbbbbbbbb, 0xcccccccc,
     0xdddddddd, 0xeeeeeeee, 0xffffffff
@@ -98,7 +98,7 @@ avx512f_test (void)
       for (j = 0; j < 10; j++)
 	{
 	  s2.a[0] = controls[j];
-	  compute_fixupimmpd (&res_ref[0], s1.a[0], s2.a[0]);
+	  compute_fixupimmpd (&res_ref[0], res1.a[0], s1.a[0], s2.a[0]);
 
 	  res1.x = _mm_fixupimm_sd (res1.x, s1.x, s2.x, 0);
 	  res2.x = _mm_mask_fixupimm_sd (res2.x, mask, s1.x, s2.x, 0);
diff --git a/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmss-2.c b/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmss-2.c
index 26f45a0..7364cc5 100644
--- a/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmss-2.c
+++ b/gcc/testsuite/gcc.target/i386/avx512f-vfixupimmss-2.c
@@ -10,12 +10,12 @@
 #include "avx512f-mask-type.h"
 
 void
-compute_fixupimmps (float *r, float src, int tbl)
+compute_fixupimmps (float *r, float dest, float src, int tbl)
 {
   switch (tbl & 0xf)
     {
     case 0:
-      *r = src;
+      *r = dest;
       break;
     case 1:
       *r = src;
@@ -76,7 +76,7 @@ avx512f_test (void)
   int i, j, k;
 
   float vals[2] = { -10, 10 };
-  int controls[10] = { 0x11111111,
+  int controls[10] = { 0,
     0x77777777, 0x88888888, 0x99999999,
     0xaaaaaaaa, 0xbbbbbbbb, 0xcccccccc,
     0xdddddddd, 0xeeeeeeee, 0xffffffff
@@ -99,7 +99,7 @@ avx512f_test (void)
       for (j = 0; j < 10; j++)
 	{
 	  s2.a[0] = controls[j];
-	  compute_fixupimmps (&res_ref[0], s1.a[0], s2.a[0]);
+	  compute_fixupimmps (&res_ref[0], res1.a[0], s1.a[0], s2.a[0]);
 
 	  res1.x = _mm_fixupimm_ss (res1.x, s1.x, s2.x, 0);
 	  res2.x = _mm_mask_fixupimm_ss (res2.x, mask, s1.x, s2.x, 0);

Re: [PATCH] x86: Revert patches to fix PR target/88794

[Jakub Jelinek]

On Wed, Jan 16, 2019 at 10:48:28PM +0800, Wei Xiao wrote:
> The original runtime testcases are incorrect and I have fixed them as attached.
> Is it ok to do the revert and fix the testcases for trunk?

LGTM.

> 2019-01-16  Wei Xiao  <wei3.xiao@intel.com>
> 
>         * gcc.target/i386/avx512f-vfixupimmpd-2.c: Fix the test cases for
>         VFIXUPIMM* intrinsics.
>         * gcc.target/i386/avx512f-vfixupimmps-2.c: Ditto.
>         * gcc.target/i386/avx512f-vfixupimmsd-2.c: Ditto.
>         * gcc.target/i386/avx512f-vfixupimmss-2.c: Ditto.

	Jakub

[PATCH] Read avx512vl-vfixupimms*-2.c testcases (PR target/88489)

[Jakub Jelinek]

On Wed, Jan 16, 2019 at 07:12:56PM +0800, Wei Xiao wrote:
> > > Yes, but please test the compiler after the revert. Please also create
> > > a runtime testcase out of the testcase in the PR.
> Yes, we have tested it but current runtime testcase can't cover the corner
> case to expose the incorrectness of SDM. We will add some after the revert.
> 
> > For r267160, I'd expect you want to revert just the config/i386/ part and
> > keep the testcases, they should work even with the changes reverted, right?
> >
> The testcase part also need to be reverted since we have changed them
> according to the incorrect intrinsic list in SDM.

I don't really understand this.

The testcases succeed just fine for me in the current trunk with all the
reversions and test something the current state of the testsuite doesn't
check normally, in particular that the testcases run correctly even when
-mavx512vl is used.  As that misbehaved in the past, we should make sure we
don't break that again.

Uros, is it ok to reapply this to current trunk?

2019-01-17  Jakub Jelinek  <jakub@redhat.com>

	Reapply:
	2018-12-15  Jakub Jelinek  <jakub@redhat.com>

	PR target/88489
	* gcc.target/i386/avx512vl-vfixupimmsd-2.c: New test.
	* gcc.target/i386/avx512vl-vfixupimmss-2.c: New test.

--- gcc/testsuite/gcc.target/i386/avx512vl-vfixupimmsd-2.c	(nonexistent)
+++ gcc/testsuite/gcc.target/i386/avx512vl-vfixupimmsd-2.c	(revision 268010)
@@ -0,0 +1,20 @@
+/* { dg-do run } */
+/* { dg-options "-mavx512vl -O2 -std=gnu99" } */
+/* { dg-require-effective-target avx512vl } */
+/* { dg-require-effective-target c99_runtime } */
+
+#define AVX512VL
+#define AVX512F_LEN 512
+#define AVX512F_LEN_HALF 256
+#include "avx512f-vfixupimmsd-2.c"
+
+static void
+test_256 (void)
+{
+  test_512 ();
+}
+
+static void
+test_128 (void)
+{
+}
--- gcc/testsuite/gcc.target/i386/avx512vl-vfixupimmss-2.c	(nonexistent)
+++ gcc/testsuite/gcc.target/i386/avx512vl-vfixupimmss-2.c	(revision 268010)
@@ -0,0 +1,20 @@
+/* { dg-do run } */
+/* { dg-options "-mavx512vl -O2 -std=gnu99" } */
+/* { dg-require-effective-target avx512vl } */
+/* { dg-require-effective-target c99_runtime } */
+
+#define AVX512VL
+#define AVX512F_LEN 512
+#define AVX512F_LEN_HALF 256
+#include "avx512f-vfixupimmss-2.c"
+
+static void
+test_256 (void)
+{
+  test_512 ();
+}
+
+static void
+test_128 (void)
+{
+}


	Jakub

Re: [PATCH] Read avx512vl-vfixupimms*-2.c testcases (PR target/88489)

[Wei Xiao]

> > > For r267160, I'd expect you want to revert just the config/i386/ part and
> > > keep the testcases, they should work even with the changes reverted, right?
> > >
> > The testcase part also need to be reverted since we have changed them
> > according to the incorrect intrinsic list in SDM.
>
> I don't really understand this.
>
> The testcases succeed just fine for me in the current trunk with all the
> reversions and test something the current state of the testsuite doesn't
> check normally, in particular that the testcases run correctly even when
> -mavx512vl is used.  As that misbehaved in the past, we should make sure we
> don't break that again.
>

You're right. The testcases need to be kept to prevent regression.

> Uros, is it ok to reapply this to current trunk?

Re: [PATCH] Read avx512vl-vfixupimms*-2.c testcases (PR target/88489)

[Uros Bizjak]

On Fri, Jan 18, 2019 at 3:42 AM Wei Xiao <wei.william.xiao@gmail.com> wrote:
>
> > > > For r267160, I'd expect you want to revert just the config/i386/ part and
> > > > keep the testcases, they should work even with the changes reverted, right?
> > > >
> > > The testcase part also need to be reverted since we have changed them
> > > according to the incorrect intrinsic list in SDM.
> >
> > I don't really understand this.
> >
> > The testcases succeed just fine for me in the current trunk with all the
> > reversions and test something the current state of the testsuite doesn't
> > check normally, in particular that the testcases run correctly even when
> > -mavx512vl is used.  As that misbehaved in the past, we should make sure we
> > don't break that again.
> >
>
> You're right. The testcases need to be kept to prevent regression.
>
> > Uros, is it ok to reapply this to current trunk?

Yes, please reapply the tests.

Thanks,
Uros.