From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2153) id 438DF3858414; Wed, 8 Sep 2021 12:09:24 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 438DF3858414 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Jakub Jelinek To: gcc-cvs@gcc.gnu.org Subject: [gcc r12-3417] i386: Fix up xorsign for AVX [PR89984] X-Act-Checkin: gcc X-Git-Author: Jakub Jelinek X-Git-Refname: refs/heads/master X-Git-Oldrev: 6576ad5add7e1891c379783316b85b88346963aa X-Git-Newrev: 7485a52551d71db2e8bbfc4c484196bcc321a1cd Message-Id: <20210908120924.438DF3858414@sourceware.org> Date: Wed, 8 Sep 2021 12:09:24 +0000 (GMT) X-BeenThere: gcc-cvs@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Sep 2021 12:09:24 -0000 https://gcc.gnu.org/g:7485a52551d71db2e8bbfc4c484196bcc321a1cd commit r12-3417-g7485a52551d71db2e8bbfc4c484196bcc321a1cd Author: Jakub Jelinek Date: Wed Sep 8 14:06:10 2021 +0200 i386: Fix up xorsign for AVX [PR89984] 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. On Wed, Sep 08, 2021 at 05:23:40PM +0800, Hongtao Liu wrote: > I'm curious why we need the post_reload splitter @xorsign3_1 > for scalar mode, can't we just expand them into and/xor operations in > the expander, just like vector modes did. Following seems to work for all the testcases I've tried (and in some generates better code than the post-reload splitter). 2021-09-08 Jakub Jelinek liuhongt PR target/89984 * config/i386/i386.md (@xorsign3_1): Remove. * config/i386/i386-expand.c (ix86_expand_xorsign): Expand right away into AND with mask and XOR, using paradoxical subregs. (ix86_split_xorsign): Remove. * config/i386/i386-protos.h (ix86_split_xorsign): Remove. * gcc.target/i386/avx-pr102224.c: Fix up PR number. * gcc.dg/pr89984.c: New test. * gcc.target/i386/avx-pr89984.c: New test. Diff: --- gcc/config/i386/i386-expand.c | 59 ++++------------------------ gcc/config/i386/i386-protos.h | 1 - gcc/config/i386/i386.md | 14 ------- gcc/testsuite/gcc.dg/pr89984.c | 20 ++++++++++ gcc/testsuite/gcc.target/i386/avx-pr102224.c | 2 +- gcc/testsuite/gcc.target/i386/avx-pr89984.c | 23 +++++++++++ 6 files changed, 51 insertions(+), 68 deletions(-) diff --git a/gcc/config/i386/i386-expand.c b/gcc/config/i386/i386-expand.c index 0cc572c4903..badbacc19d8 100644 --- a/gcc/config/i386/i386-expand.c +++ b/gcc/config/i386/i386-expand.c @@ -2270,7 +2270,7 @@ void ix86_expand_xorsign (rtx operands[]) { machine_mode mode, vmode; - rtx dest, op0, op1, mask; + rtx dest, op0, op1, mask, x, temp; dest = operands[0]; op0 = operands[1]; @@ -2285,60 +2285,15 @@ ix86_expand_xorsign (rtx operands[]) else gcc_unreachable (); + temp = gen_reg_rtx (vmode); mask = ix86_build_signbit_mask (vmode, 0, 0); - emit_insn (gen_xorsign3_1 (mode, dest, op0, op1, mask)); -} - -/* Deconstruct an xorsign operation into bit masks. */ - -void -ix86_split_xorsign (rtx operands[]) -{ - machine_mode mode, vmode; - rtx dest, op0, op1, mask, x; - - dest = operands[0]; - op0 = operands[1]; - op1 = operands[2]; - mask = operands[3]; - - 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 (temp, 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, temp, op0); dest = lowpart_subreg (vmode, dest, mode); emit_insn (gen_rtx_SET (dest, x)); diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h index 355df116439..72644e33a92 100644 --- a/gcc/config/i386/i386-protos.h +++ b/gcc/config/i386/i386-protos.h @@ -138,7 +138,6 @@ extern void ix86_expand_copysign (rtx []); extern void ix86_split_copysign_const (rtx []); extern void ix86_split_copysign_var (rtx []); extern void ix86_expand_xorsign (rtx []); -extern void ix86_split_xorsign (rtx []); extern bool ix86_unary_operator_ok (enum rtx_code, machine_mode, rtx[]); extern bool ix86_match_ccmode (rtx, machine_mode); extern void ix86_expand_branch (enum rtx_code, rtx, rtx, rtx); diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index 0414f24949e..6b4ceb2bce3 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -10917,20 +10917,6 @@ ix86_expand_xorsign (operands); DONE; }) - -(define_insn_and_split "@xorsign3_1" - [(set (match_operand:MODEF 0 "register_operand" "=&Yv,&Yv,&Yv") - (unspec:MODEF - [(match_operand:MODEF 1 "register_operand" "Yv,0,Yv") - (match_operand:MODEF 2 "register_operand" "0,Yv,Yv") - (match_operand: 3 "nonimmediate_operand" "Yvm,Yvm,Yvm")] - UNSPEC_XORSIGN))] - "SSE_FLOAT_MODE_P (mode) && TARGET_SSE_MATH" - "#" - "&& reload_completed" - [(const_int 0)] - "ix86_split_xorsign (operands); DONE;" - [(set_attr "isa" "*,avx,avx")]) ;; One complement instructions diff --git a/gcc/testsuite/gcc.dg/pr89984.c b/gcc/testsuite/gcc.dg/pr89984.c new file mode 100644 index 00000000000..471fe92bc86 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr89984.c @@ -0,0 +1,20 @@ +/* PR target/89984 */ +/* { dg-do run } */ +/* { dg-options "-O2" } */ + +__attribute__((noipa)) float +foo (float x, float y) +{ + return x * __builtin_copysignf (1.0f, y) + y; +} + +int +main () +{ + if (foo (1.25f, 7.25f) != 1.25f + 7.25f + || foo (1.75f, -3.25f) != -1.75f + -3.25f + || foo (-2.25f, 7.5f) != -2.25f + 7.5f + || foo (-3.0f, -4.0f) != 3.0f + -4.0f) + __builtin_abort (); + return 0; +} diff --git a/gcc/testsuite/gcc.target/i386/avx-pr102224.c b/gcc/testsuite/gcc.target/i386/avx-pr102224.c index be6b88c05db..7cb8b4cdecb 100644 --- a/gcc/testsuite/gcc.target/i386/avx-pr102224.c +++ b/gcc/testsuite/gcc.target/i386/avx-pr102224.c @@ -1,4 +1,4 @@ -/* PR tree-optimization/51581 */ +/* PR target/102224 */ /* { dg-do run } */ /* { dg-options "-O2 -mavx" } */ /* { dg-require-effective-target avx } */ diff --git a/gcc/testsuite/gcc.target/i386/avx-pr89984.c b/gcc/testsuite/gcc.target/i386/avx-pr89984.c new file mode 100644 index 00000000000..3409adef5b6 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/avx-pr89984.c @@ -0,0 +1,23 @@ +/* PR target/89984 */ +/* { 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/pr89984.c" +#undef main + +#include CHECK_H + +static void +TEST (void) +{ + main1 (); +}