Hi!
As the testcase shows, we miscompile @xorsign<mode>3_1 if both input
operands are in the same register, because the splitter overwrites op1
before with op1 & mask before using op0.
For dest = xorsign op0, op0 we can actually simplify it from
dest = (op0 & mask) ^ op0 to dest = op0 & ~mask (aka abs).
The expander change is an optimization improvement, if we at expansion
time know it is xorsign op0, op0, we can emit abs right away and get better
code through that.
The @xorsign<mode>3_1 is a fix for the case where xorsign wouldn't be known
to have same operands during expansion, but during RTL optimizations they
would appear. For non-AVX we need to use earlyclobber, we require
dest and op1 to be the same but op0 must be different because we overwrite
op1 first. For AVX the constraints ensure that at most 2 of the 3 operands
may be the same register and if both inputs are the same, handles that case.
This case can be easily tested with the xorsign<mode>3 expander change
reverted.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Thinking about it more this morning, while this patch fixes the problems
revealed in the testcase, the recent PR89984 change was buggy too, but
perhaps that can be fixed incrementally. Because for AVX the new code
destructively modifies op1. If that is different from dest, say on:
float
foo (float x, float y)
{
return x * __builtin_copysignf (1.0f, y) + y;
}
then we get after RA:
(insn 8 7 9 2 (set (reg:SF 20 xmm0 [orig:82 _2 ] [82])
(unspec:SF [
(reg:SF 20 xmm0 [88])
(reg:SF 21 xmm1 [89])
(mem/u/c:V4SF (symbol_ref/u:DI ("*.LC0") [flags 0x2]) [0 S16
A128])
] UNSPEC_XORSIGN)) "hohoho.c":4:12 649 {xorsignsf3_1}
(nil))
(insn 9 8 15 2 (set (reg:SF 20 xmm0 [87])
(plus:SF (reg:SF 20 xmm0 [orig:82 _2 ] [82])
(reg:SF 21 xmm1 [89]))) "hohoho.c":4:44 1021 {*fop_sf_comm}
(nil))
but split the xorsign into:
vandps .LC0(%rip), %xmm1, %xmm1
vxorps %xmm0, %xmm1, %xmm0
and then the addition:
vaddss %xmm1, %xmm0, %xmm0
which means we miscompile it - instead of adding y in the end we add
__builtin_copysignf (0.0f, y).
So, wonder if we don't want instead in addition to the &Yv <- Yv, 0
alternative (enabled for both pre-AVX and AVX as in this patch) the
&Yv <- Yv, Yv where destination must be different from inputs and another
Yv <- Yv, Yv where it can be the same but then need a match_scratch
(with X for the other alternatives and =Yv for the last one).
That way we'd always have a safe register we can store the op1 & mask
value into, either the destination (in the first alternative known to
be equal to op1 which is needed for non-AVX but ok for AVX too), in the
second alternative known to be different from both inputs and in the third
which could be used for those
float bar (float x, float y) { return x * __builtin_copysignf (1.0f, y); }
cases where op1 is naturally xmm1 and dest == op0 naturally xmm0 we'd use
some other register like xmm2.
2021-09-07 Jakub Jelinek <ja...@redhat.com>
PR target/102224
* config/i386/i386.md (xorsign<mode>3): If operands[1] is equal to
operands[2], emit abs<mode>2 instead.
(@xorsign<mode>3_1): Add early-clobbers for output operand, enable
first alternative even for avx, add another alternative with
=&Yv <- 0, Yv, Yvm constraints.
* config/i386/i386-expand.c (ix86_split_xorsign): If op0 is equal
to op1, emit vpandn instead.
* gcc.dg/pr102224.c: New test.
* gcc.target/i386/avx-pr102224.c: New test.
--- gcc/config/i386/i386.md.jj 2021-09-06 14:47:43.199049975 +0200
+++ gcc/config/i386/i386.md 2021-09-07 13:13:50.825603852 +0200
@@ -10803,21 +10803,27 @@ (define_expand "xorsign<mode>3"
(match_operand:MODEF 1 "register_operand")
(match_operand:MODEF 2 "register_operand")]
"SSE_FLOAT_MODE_P (<MODE>mode) && TARGET_SSE_MATH"
- "ix86_expand_xorsign (operands); DONE;")
+{
+ if (rtx_equal_p (operands[1], operands[2]))
+ emit_insn (gen_abs<mode>2 (operands[0], operands[1]));
+ else
+ ix86_expand_xorsign (operands);
+ DONE;
+})
(define_insn_and_split "@xorsign<mode>3_1"
- [(set (match_operand:MODEF 0 "register_operand" "=Yv,Yv")
+ [(set (match_operand:MODEF 0 "register_operand" "=&Yv,&Yv,&Yv")
(unspec:MODEF
- [(match_operand:MODEF 1 "register_operand" "Yv,Yv")
- (match_operand:MODEF 2 "register_operand" "0,Yv")
- (match_operand:<ssevecmode> 3 "nonimmediate_operand" "Yvm,Yvm")]
+ [(match_operand:MODEF 1 "register_operand" "Yv,0,Yv")
+ (match_operand:MODEF 2 "register_operand" "0,Yv,Yv")
+ (match_operand:<ssevecmode> 3 "nonimmediate_operand" "Yvm,Yvm,Yvm")]
UNSPEC_XORSIGN))]
"SSE_FLOAT_MODE_P (<MODE>mode) && TARGET_SSE_MATH"
"#"
"&& reload_completed"
[(const_int 0)]
"ix86_split_xorsign (operands); DONE;"
- [(set_attr "isa" "noavx,avx")])
+ [(set_attr "isa" "*,avx,avx")])
;; One complement instructions
--- gcc/config/i386/i386-expand.c.jj 2021-09-07 12:35:26.340685935 +0200
+++ gcc/config/i386/i386-expand.c 2021-09-07 13:09:43.961032052 +0200
@@ -2289,12 +2289,39 @@ ix86_split_xorsign (rtx operands[])
mode = GET_MODE (dest);
vmode = GET_MODE (mask);
- op1 = lowpart_subreg (vmode, op1, mode);
- x = gen_rtx_AND (vmode, op1, mask);
- emit_insn (gen_rtx_SET (op1, x));
+ /* The constraints ensure that for non-AVX dest == op1 is
+ different from op0, and for AVX that at most two of
+ dest, op0 and op1 are the same register but the third one
+ is different. */
+ if (rtx_equal_p (op0, op1))
+ {
+ gcc_assert (TARGET_AVX && !rtx_equal_p (op0, dest));
+ if (vmode == V4SFmode)
+ vmode = V4SImode;
+ else
+ {
+ gcc_assert (vmode == V2DFmode);
+ vmode = V2DImode;
+ }
+ mask = lowpart_subreg (vmode, mask, GET_MODE (mask));
+ if (MEM_P (mask))
+ {
+ rtx msk = lowpart_subreg (vmode, dest, mode);
+ emit_insn (gen_rtx_SET (msk, mask));
+ mask = msk;
+ }
+ op0 = lowpart_subreg (vmode, op0, mode);
+ x = gen_rtx_AND (vmode, gen_rtx_NOT (vmode, mask), op0);
+ }
+ else
+ {
+ op1 = lowpart_subreg (vmode, op1, mode);
+ x = gen_rtx_AND (vmode, op1, mask);
+ emit_insn (gen_rtx_SET (op1, x));
- op0 = lowpart_subreg (vmode, op0, mode);
- x = gen_rtx_XOR (vmode, op1, op0);
+ op0 = lowpart_subreg (vmode, op0, mode);
+ x = gen_rtx_XOR (vmode, op1, op0);
+ }
dest = lowpart_subreg (vmode, dest, mode);
emit_insn (gen_rtx_SET (dest, x));
--- gcc/testsuite/gcc.dg/pr102224.c.jj 2021-09-07 12:17:43.088507018 +0200
+++ gcc/testsuite/gcc.dg/pr102224.c 2021-09-07 13:11:52.923241157 +0200
@@ -0,0 +1,49 @@
+/* PR target/102224 */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+__attribute__((noipa)) float
+foo (float x)
+{
+ return x * __builtin_copysignf (1.0f, x);
+}
+
+__attribute__((noipa)) float
+bar (float x, float y)
+{
+ return x * __builtin_copysignf (1.0f, y);
+}
+
+__attribute__((noipa)) float
+baz (float z, float x)
+{
+ return x * __builtin_copysignf (1.0f, x);
+}
+
+__attribute__((noipa)) float
+qux (float z, float x, float y)
+{
+ return x * __builtin_copysignf (1.0f, y);
+}
+
+int
+main ()
+{
+ if (foo (1.0f) != 1.0f
+ || foo (-4.0f) != 4.0f)
+ __builtin_abort ();
+ if (bar (1.25f, 7.25f) != 1.25f
+ || bar (1.75f, -3.25f) != -1.75f
+ || bar (-2.25f, 7.5f) != -2.25f
+ || bar (-3.0f, -4.0f) != 3.0f)
+ __builtin_abort ();
+ if (baz (5.5f, 1.0f) != 1.0f
+ || baz (4.25f, -4.0f) != 4.0f)
+ __builtin_abort ();
+ if (qux (1.0f, 1.25f, 7.25f) != 1.25f
+ || qux (2.0f, 1.75f, -3.25f) != -1.75f
+ || qux (3.0f, -2.25f, 7.5f) != -2.25f
+ || qux (4.0f, -3.0f, -4.0f) != 3.0f)
+ __builtin_abort ();
+ return 0;
+}
--- gcc/testsuite/gcc.target/i386/avx-pr102224.c.jj 2021-09-07
13:12:41.429567551 +0200
+++ gcc/testsuite/gcc.target/i386/avx-pr102224.c 2021-09-07
13:13:11.397151393 +0200
@@ -0,0 +1,23 @@
+/* PR tree-optimization/51581 */
+/* { dg-do run } */
+/* { dg-options "-O2 -mavx" } */
+/* { dg-require-effective-target avx } */
+
+#ifndef CHECK_H
+#define CHECK_H "avx-check.h"
+#endif
+#ifndef TEST
+#define TEST avx_test
+#endif
+
+#define main main1
+#include "../../gcc.dg/pr102224.c"
+#undef main
+
+#include CHECK_H
+
+static void
+TEST (void)
+{
+ main1 ();
+}
Jakub
