This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

[PATCH] Fix wrong-code x86 issue with avx512{f,vl} fma (PR target/93009)

From: Jakub Jelinek <jakub at redhat dot com>
To: Uros Bizjak <ubizjak at gmail dot com>, Jeff Law <law at redhat dot com>
Cc: gcc-patches at gcc dot gnu dot org, hjl dot tools at gmail dot com
Date: Sat, 21 Dec 2019 00:34:54 +0100
Subject: [PATCH] Fix wrong-code x86 issue with avx512{f,vl} fma (PR target/93009)
Reply-to: Jakub Jelinek <jakub at redhat dot com>

Hi!

As mentioned in the PR, the following testcase is miscompiled with avx512vl.
The reason is that the fma *_bcst_1 define_insns have two alternatives:
"=v,v" "0,v" "v,0" "m,m" and use the same
vfmadd213* %3<avx512bcst>, %2, %0<sd_mask_op4>
pattern.  If the first alternative is chosen, everything is ok, but if the
second alternative is chosen, %2 and %0 are the same register, so instead
of doing dest=dest*another+membcst we do dest=dest*dest+membcst.
Now, to fix this, either we'd need separate:
  "vfmadd213<ssemodesuffix>\t{%3<avx512bcst>, %2, %0<sd_mask_op4>|%0<sd_mask_op4>, %2, %3<avx512bcst>}
   vfmadd213<ssemodesuffix>\t{%3<avx512bcst>, %1, %0<sd_mask_op4>|%0<sd_mask_op4>, %1, %3<avx512bcst>}"
where for the second alternative, we'd just use %1 instead of %2, but
what I think is actually cleaner is just use a single alternative and
make the two multiplication operands commutative, which they really are.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-12-20  Jakub Jelinek  <jakub@redhat.com>

	PR target/93009
	* config/i386/sse.md
	(*<sd_mask_codefor>fma_fmadd_<mode><sd_maskz_name>_bcst_1,
	*<sd_mask_codefor>fma_fmsub_<mode><sd_maskz_name>_bcst_1,
	*<sd_mask_codefor>fma_fnmadd_<mode><sd_maskz_name>_bcst_1,
	*<sd_mask_codefor>fma_fnmsub_<mode><sd_maskz_name>_bcst_1): Use
	just a single alternative instead of two, make operands 1 and 2
	commutative.

	* gcc.target/i386/avx512vl-pr93009.c: New test.

--- gcc/config/i386/sse.md.jj	2019-12-09 11:12:30.000000000 +0100
+++ gcc/config/i386/sse.md	2019-12-20 21:44:58.165432773 +0100
@@ -4172,12 +4172,12 @@ (define_insn "<sd_mask_codefor>fma_fmadd
    (set_attr "mode" "<MODE>")])
 
 (define_insn "*<sd_mask_codefor>fma_fmadd_<mode><sd_maskz_name>_bcst_1"
-  [(set (match_operand:VF_AVX512 0 "register_operand" "=v,v")
+  [(set (match_operand:VF_AVX512 0 "register_operand" "=v")
 	(fma:VF_AVX512
-	  (match_operand:VF_AVX512 1 "register_operand" "0,v")
-	  (match_operand:VF_AVX512 2 "register_operand" "v,0")
+	  (match_operand:VF_AVX512 1 "register_operand" "%0")
+	  (match_operand:VF_AVX512 2 "register_operand" "v")
 	  (vec_duplicate:VF_AVX512
-	    (match_operand:<ssescalarmode> 3 "memory_operand" "m,m"))))]
+	    (match_operand:<ssescalarmode> 3 "memory_operand" "m"))))]
   "TARGET_AVX512F && <sd_mask_mode512bit_condition>"
   "vfmadd213<ssemodesuffix>\t{%3<avx512bcst>, %2, %0<sd_mask_op4>|%0<sd_mask_op4>, %2, %3<avx512bcst>}"
   [(set_attr "type" "ssemuladd")
@@ -4289,13 +4289,13 @@ (define_insn "<sd_mask_codefor>fma_fmsub
    (set_attr "mode" "<MODE>")])
 
 (define_insn "*<sd_mask_codefor>fma_fmsub_<mode><sd_maskz_name>_bcst_1"
-  [(set (match_operand:VF_AVX512 0 "register_operand" "=v,v")
+  [(set (match_operand:VF_AVX512 0 "register_operand" "=v")
 	(fma:VF_AVX512
-	  (match_operand:VF_AVX512 1 "register_operand" "0,v")
-	  (match_operand:VF_AVX512 2 "register_operand" "v,0")
+	  (match_operand:VF_AVX512 1 "register_operand" "%0")
+	  (match_operand:VF_AVX512 2 "register_operand" "v")
 	  (neg:VF_AVX512
 	    (vec_duplicate:VF_AVX512
-	      (match_operand:<ssescalarmode> 3 "memory_operand" "m,m")))))]
+	      (match_operand:<ssescalarmode> 3 "memory_operand" "m")))))]
   "TARGET_AVX512F && <sd_mask_mode512bit_condition>"
   "vfmsub213<ssemodesuffix>\t{%3<avx512bcst>, %2, %0<sd_mask_op4>|%0<sd_mask_op4>, %2, %3<avx512bcst>}"
   [(set_attr "type" "ssemuladd")
@@ -4411,13 +4411,13 @@ (define_insn "<sd_mask_codefor>fma_fnmad
    (set_attr "mode" "<MODE>")])
 
 (define_insn "*<sd_mask_codefor>fma_fnmadd_<mode><sd_maskz_name>_bcst_1"
-  [(set (match_operand:VF_AVX512 0 "register_operand" "=v,v")
+  [(set (match_operand:VF_AVX512 0 "register_operand" "=v")
 	(fma:VF_AVX512
 	  (neg:VF_AVX512
-	    (match_operand:VF_AVX512 1 "register_operand" "0,v"))
-	  (match_operand:VF_AVX512 2 "register_operand" "v,0")
+	    (match_operand:VF_AVX512 1 "register_operand" "%0"))
+	  (match_operand:VF_AVX512 2 "register_operand" "v")
 	  (vec_duplicate:VF_AVX512
-	    (match_operand:<ssescalarmode> 3 "memory_operand" "m,m"))))]
+	    (match_operand:<ssescalarmode> 3 "memory_operand" "m"))))]
   "TARGET_AVX512F && <sd_mask_mode512bit_condition>"
   "vfnmadd213<ssemodesuffix>\t{%3<avx512bcst>, %2, %0<sd_mask_op4>|%0<sd_mask_op4>, %2, %3<avx512bcst>}"
   [(set_attr "type" "ssemuladd")
@@ -4535,14 +4535,14 @@ (define_insn "<sd_mask_codefor>fma_fnmsu
    (set_attr "mode" "<MODE>")])
 
 (define_insn "*<sd_mask_codefor>fma_fnmsub_<mode><sd_maskz_name>_bcst_1"
-  [(set (match_operand:VF_AVX512 0 "register_operand" "=v,v")
+  [(set (match_operand:VF_AVX512 0 "register_operand" "=v")
 	(fma:VF_AVX512
 	  (neg:VF_AVX512
-	    (match_operand:VF_AVX512 1 "register_operand" "0,v"))
-	  (match_operand:VF_AVX512 2 "register_operand" "v,0")
+	    (match_operand:VF_AVX512 1 "register_operand" "%0"))
+	  (match_operand:VF_AVX512 2 "register_operand" "v")
 	  (neg:VF_AVX512
 	    (vec_duplicate:VF_AVX512
-	      (match_operand:<ssescalarmode> 3 "memory_operand" "m,m")))))]
+	      (match_operand:<ssescalarmode> 3 "memory_operand" "m")))))]
   "TARGET_AVX512F && <sd_mask_mode512bit_condition>"
   "vfnmsub213<ssemodesuffix>\t{%3<avx512bcst>, %2, %0<sd_mask_op4>|%0<sd_mask_op4>, %2, %3<avx512bcst>}"
   [(set_attr "type" "ssemuladd")
--- gcc/testsuite/gcc.target/i386/avx512vl-pr93009.c.jj	2019-12-20 21:54:11.158011954 +0100
+++ gcc/testsuite/gcc.target/i386/avx512vl-pr93009.c	2019-12-20 21:53:50.375328425 +0100
@@ -0,0 +1,38 @@
+/* PR target/93009 */
+/* { dg-do run { target { avx512vl } } } */
+/* { dg-options "-O2 -mavx512vl" } */
+
+#define AVX512VL
+#define AVX512F_LEN 512
+#define AVX512F_LEN_HALF 256
+
+#include "avx512f-check.h"
+
+typedef double v2df __attribute__((vector_size (16)));
+
+__attribute__((noipa)) v2df
+foo (v2df x, v2df y, double *z)
+{
+  return x * y + (v2df) { z[0], z[0] };
+}
+
+__attribute__((noipa)) v2df
+bar (v2df x, v2df y, double *z)
+{
+  return y * x + (v2df) { z[0], z[0] };
+}
+
+static void
+test_256 (void)
+{
+}
+
+static void
+test_128 (void)
+{
+  double z = 5.0;
+  v2df x = foo ((v2df) { 1.0, 2.0 }, (v2df) { 3.0, 4.0 }, &z);
+  v2df y = bar ((v2df) { 6.0, 7.0 }, (v2df) { 8.0, 9.0 }, &z);
+  if (x[0] != 8.0 || x[1] != 13.0 || y[0] != 53.0 || y[1] != 68.0)
+    abort ();
+}

	Jakub

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]