From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) by sourceware.org (Postfix) with ESMTPS id 483133858D39 for ; Wed, 19 Apr 2023 08:43:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 483133858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-wm1-x336.google.com with SMTP id 5b1f17b1804b1-3f0a0c4e1ebso31648155e9.3 for ; Wed, 19 Apr 2023 01:43:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1681893790; x=1684485790; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=ADM5ioYWaQOrj94D27EFZvnyPWK8LIqH3USmMImxYmY=; b=SLN+CUmtxBpWwk4rCyngzsl+sc8jzSHJLOXAwTvpvckLZsrKkusV88x/Z4XZMpAQlV KAcwIqrKBryAMYCacY9LUUcsBy9MOko+aKN2EtASTdbDqRXHLWHQ1yE6CArgPo757KwF Weyb/8quuQAW04Sd0JiKRJ2z3nA4Pibt+YaZIFyRLyul3b7B1D8Awg8DAyWJHqf0X7Ze PamQ8ETNfGawfXCw4NVOLO33S2TEOBWtSIhOARlKlGQcJQrnHKABa3aoevwiagk9gtlE aQbsUXQT19wYmi5UentHCLuJuqr/iZrGp2X1h8O9Rzv0yA3HQHLf2sVcdi+WBRVmC02N aFBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681893790; x=1684485790; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ADM5ioYWaQOrj94D27EFZvnyPWK8LIqH3USmMImxYmY=; b=YWX1R/GSClv+WXX9GxVZCID7J02R7MnnyvwfM2nuxGSmfQdtXgMwGCattiinR0WMcd RX6ctAveRQ7XtfUA+IjY1BiL8E7HzvUcOUa/ILtDI2wembEv538UhsX8uNEpxOlf/Htd ykYGTzW2/2i8h6fNWaCSFTcQwwWOp6x+SJzeSCmRgy2Pww90jTrZ4vl07muQAusg9l2e 1JDfAfiRac23rWsYTg9kyhN3fFrDU9f+kqYQntzYGvCtqbrQmfYypp6jfz5F6BnlfDfA DMTCWHeSRRIKUPzuebX6Tx7IFs9DUZrRJb9+NOmEs7ksU+3H50bP+CwP12tpxNW62Xsj Hltg== X-Gm-Message-State: AAQBX9eiiebQCMtSXItqtXVLSu25wyIgQJVmUimun9t2lDFlOtiS1EBa QF0tYWle86nQiIh5IAjVKWDw0yegSOANmBIR2+6/H3ttkvrmouNbIEk= X-Google-Smtp-Source: AKy350Z5p9IlPJx5JiIicHsLFENywyyicNCzlYBYvbITm0XoIEqn2f4pO6SnJNi2CTN++KxO0VgyKb86tfiHurXqpL0= X-Received: by 2002:adf:dece:0:b0:2fb:600e:55bd with SMTP id i14-20020adfdece000000b002fb600e55bdmr3944806wrn.39.1681893789805; Wed, 19 Apr 2023 01:43:09 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Prathamesh Kulkarni Date: Wed, 19 Apr 2023 14:12:33 +0530 Message-ID: Subject: Re: [aarch64] Use wzr/xzr for assigning vector element to 0 To: Prathamesh Kulkarni , gcc Patches , richard.sandiford@arm.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_NUMSUBJECT,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, 31 Jan 2023 at 11:51, Richard Sandiford wrote: > > Prathamesh Kulkarni writes: > > On Mon, 23 Jan 2023 at 22:26, Richard Sandiford > > wrote: > >> > >> Prathamesh Kulkarni writes: > >> > On Wed, 18 Jan 2023 at 19:59, Richard Sandiford > >> > wrote: > >> >> > >> >> Prathamesh Kulkarni writes: > >> >> > On Tue, 17 Jan 2023 at 18:29, Richard Sandiford > >> >> > wrote: > >> >> >> > >> >> >> Prathamesh Kulkarni writes: > >> >> >> > Hi Richard, > >> >> >> > For the following (contrived) test: > >> >> >> > > >> >> >> > void foo(int32x4_t v) > >> >> >> > { > >> >> >> > v[3] = 0; > >> >> >> > return v; > >> >> >> > } > >> >> >> > > >> >> >> > -O2 code-gen: > >> >> >> > foo: > >> >> >> > fmov s1, wzr > >> >> >> > ins v0.s[3], v1.s[0] > >> >> >> > ret > >> >> >> > > >> >> >> > I suppose we can instead emit the following code-gen ? > >> >> >> > foo: > >> >> >> > ins v0.s[3], wzr > >> >> >> > ret > >> >> >> > > >> >> >> > combine produces: > >> >> >> > Failed to match this instruction: > >> >> >> > (set (reg:V4SI 95 [ v ]) > >> >> >> > (vec_merge:V4SI (const_vector:V4SI [ > >> >> >> > (const_int 0 [0]) repeated x4 > >> >> >> > ]) > >> >> >> > (reg:V4SI 97) > >> >> >> > (const_int 8 [0x8]))) > >> >> >> > > >> >> >> > So, I wrote the following pattern to match the above insn: > >> >> >> > (define_insn "aarch64_simd_vec_set_zero" > >> >> >> > [(set (match_operand:VALL_F16 0 "register_operand" "=w") > >> >> >> > (vec_merge:VALL_F16 > >> >> >> > (match_operand:VALL_F16 1 "const_dup0_operand" "w") > >> >> >> > (match_operand:VALL_F16 3 "register_operand" "0") > >> >> >> > (match_operand:SI 2 "immediate_operand" "i")))] > >> >> >> > "TARGET_SIMD" > >> >> >> > { > >> >> >> > int elt = ENDIAN_LANE_N (, exact_log2 (INTVAL (operands[2]))); > >> >> >> > operands[2] = GEN_INT ((HOST_WIDE_INT) 1 << elt); > >> >> >> > return "ins\\t%0.[%p2], wzr"; > >> >> >> > } > >> >> >> > ) > >> >> >> > > >> >> >> > which now matches the above insn produced by combine. > >> >> >> > However, in reload dump, it creates a new insn for assigning > >> >> >> > register to (const_vector (const_int 0)), > >> >> >> > which results in: > >> >> >> > (insn 19 8 13 2 (set (reg:V4SI 33 v1 [99]) > >> >> >> > (const_vector:V4SI [ > >> >> >> > (const_int 0 [0]) repeated x4 > >> >> >> > ])) "wzr-test.c":8:1 1269 {*aarch64_simd_movv4si} > >> >> >> > (nil)) > >> >> >> > (insn 13 19 14 2 (set (reg/i:V4SI 32 v0) > >> >> >> > (vec_merge:V4SI (reg:V4SI 33 v1 [99]) > >> >> >> > (reg:V4SI 32 v0 [97]) > >> >> >> > (const_int 8 [0x8]))) "wzr-test.c":8:1 1808 > >> >> >> > {aarch64_simd_vec_set_zerov4si} > >> >> >> > (nil)) > >> >> >> > > >> >> >> > and eventually the code-gen: > >> >> >> > foo: > >> >> >> > movi v1.4s, 0 > >> >> >> > ins v0.s[3], wzr > >> >> >> > ret > >> >> >> > > >> >> >> > To get rid of redundant assignment of 0 to v1, I tried to split the > >> >> >> > above pattern > >> >> >> > as in the attached patch. This works to emit code-gen: > >> >> >> > foo: > >> >> >> > ins v0.s[3], wzr > >> >> >> > ret > >> >> >> > > >> >> >> > However, I am not sure if this is the right approach. Could you suggest, > >> >> >> > if it'd be possible to get rid of UNSPEC_SETZERO in the patch ? > >> >> >> > >> >> >> The problem is with the "w" constraint on operand 1, which tells LRA > >> >> >> to force the zero into an FPR. It should work if you remove the > >> >> >> constraint. > >> >> > Ah indeed, sorry about that, changing the constrained works. > >> >> > >> >> "i" isn't right though, because that's for scalar integers. > >> >> There's no need for any constraint here -- the predicate does > >> >> all of the work. > >> >> > >> >> > Does the attached patch look OK after bootstrap+test ? > >> >> > Since we're in stage-4, shall it be OK to commit now, or queue it for stage-1 ? > >> >> > >> >> It needs tests as well. :-) > >> >> > >> >> Also: > >> >> > >> >> > Thanks, > >> >> > Prathamesh > >> >> > > >> >> > > >> >> >> > >> >> >> Also, I think you'll need to use zr for the zero, so that > >> >> >> it uses xzr for 64-bit elements. > >> >> >> > >> >> >> I think this and the existing patterns ought to test > >> >> >> exact_log2 (INTVAL (operands[2])) >= 0 in the insn condition, > >> >> >> since there's no guarantee that RTL optimisations won't form > >> >> >> vec_merges that have other masks. > >> >> >> > >> >> >> Thanks, > >> >> >> Richard > >> >> > > >> >> > [aarch64] Use wzr/xzr for assigning 0 to vector element. > >> >> > > >> >> > gcc/ChangeLog: > >> >> > * config/aaarch64/aarch64-simd.md (aarch64_simd_vec_set_zero): > >> >> > New pattern. > >> >> > * config/aarch64/predicates.md (const_dup0_operand): New. > >> >> > > >> >> > diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md > >> >> > index 104088f67d2..8e54ee4e886 100644 > >> >> > --- a/gcc/config/aarch64/aarch64-simd.md > >> >> > +++ b/gcc/config/aarch64/aarch64-simd.md > >> >> > @@ -1083,6 +1083,20 @@ > >> >> > [(set_attr "type" "neon_ins, neon_from_gp, neon_load1_one_lane")] > >> >> > ) > >> >> > > >> >> > +(define_insn "aarch64_simd_vec_set_zero" > >> >> > + [(set (match_operand:VALL_F16 0 "register_operand" "=w") > >> >> > + (vec_merge:VALL_F16 > >> >> > + (match_operand:VALL_F16 1 "const_dup0_operand" "i") > >> >> > + (match_operand:VALL_F16 3 "register_operand" "0") > >> >> > + (match_operand:SI 2 "immediate_operand" "i")))] > >> >> > + "TARGET_SIMD && exact_log2 (INTVAL (operands[2])) >= 0" > >> >> > + { > >> >> > + int elt = ENDIAN_LANE_N (, exact_log2 (INTVAL (operands[2]))); > >> >> > + operands[2] = GEN_INT ((HOST_WIDE_INT) 1 << elt); > >> >> > + return "ins\\t%0.[%p2], zr"; > >> >> > + } > >> >> > +) > >> >> > + > >> >> > (define_insn "@aarch64_simd_vec_copy_lane" > >> >> > [(set (match_operand:VALL_F16 0 "register_operand" "=w") > >> >> > (vec_merge:VALL_F16 > >> >> > diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md > >> >> > index ff7f73d3f30..901fa1bd7f9 100644 > >> >> > --- a/gcc/config/aarch64/predicates.md > >> >> > +++ b/gcc/config/aarch64/predicates.md > >> >> > @@ -49,6 +49,13 @@ > >> >> > return CONST_INT_P (op) && IN_RANGE (INTVAL (op), 1, 3); > >> >> > }) > >> >> > > >> >> > +(define_predicate "const_dup0_operand" > >> >> > + (match_code "const_vector") > >> >> > +{ > >> >> > + op = unwrap_const_vec_duplicate (op); > >> >> > + return CONST_INT_P (op) && rtx_equal_p (op, const0_rtx); > >> >> > +}) > >> >> > + > >> >> > >> >> We already have aarch64_simd_imm_zero for this. aarch64_simd_imm_zero > >> >> is actually more general, because it works for floating-point modes too. > >> >> > >> >> I think the tests should cover all modes included in VALL_F16, since > >> >> that should have picked up this and the xzr thing. > >> > Hi Richard, > >> > Thanks for the suggestions. Does the attached patch look OK ? > >> > I am not sure how to test for v4bf and v8bf since it seems the compiler > >> > refuses conversions to/from bfloat16_t ? > >> > > >> > Thanks, > >> > Prathamesh > >> > > >> >> > >> >> Thanks, > >> >> Richard > >> >> > >> >> > (define_predicate "subreg_lowpart_operator" > >> >> > (ior (match_code "truncate") > >> >> > (and (match_code "subreg") > >> > > >> > [aarch64] Use wzr/xzr for assigning 0 to vector element. > >> > > >> > gcc/ChangeLog: > >> > * config/aaarch64/aarch64-simd.md (aarch64_simd_vec_set_zero): > >> > New pattern. > >> > > >> > gcc/testsuite/ChangeLog: > >> > * gcc.target/aarch64/vec-set-zero.c: New test. > >> > > >> > diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md > >> > index 7f212bf37cd..7428e74beaf 100644 > >> > --- a/gcc/config/aarch64/aarch64-simd.md > >> > +++ b/gcc/config/aarch64/aarch64-simd.md > >> > @@ -1083,6 +1083,20 @@ > >> > [(set_attr "type" "neon_ins, neon_from_gp, neon_load1_one_lane")] > >> > ) > >> > > >> > +(define_insn "aarch64_simd_vec_set_zero" > >> > + [(set (match_operand:VALL_F16 0 "register_operand" "=w") > >> > + (vec_merge:VALL_F16 > >> > + (match_operand:VALL_F16 1 "aarch64_simd_imm_zero" "") > >> > + (match_operand:VALL_F16 3 "register_operand" "0") > >> > + (match_operand:SI 2 "immediate_operand" "i")))] > >> > + "TARGET_SIMD && exact_log2 (INTVAL (operands[2])) >= 0" > >> > + { > >> > + int elt = ENDIAN_LANE_N (, exact_log2 (INTVAL (operands[2]))); > >> > + operands[2] = GEN_INT ((HOST_WIDE_INT) 1 << elt); > >> > + return "ins\\t%0.[%p2], zr"; > >> > + } > >> > +) > >> > + > >> > (define_insn "@aarch64_simd_vec_copy_lane" > >> > [(set (match_operand:VALL_F16 0 "register_operand" "=w") > >> > (vec_merge:VALL_F16 > >> > diff --git a/gcc/testsuite/gcc.target/aarch64/vec-set-zero.c b/gcc/testsuite/gcc.target/aarch64/vec-set-zero.c > >> > new file mode 100644 > >> > index 00000000000..c260cc9e445 > >> > --- /dev/null > >> > +++ b/gcc/testsuite/gcc.target/aarch64/vec-set-zero.c > >> > @@ -0,0 +1,32 @@ > >> > +/* { dg-do compile } */ > >> > +/* { dg-options "-O2" } */ > >> > + > >> > +#include "arm_neon.h" > >> > + > >> > +#define FOO(type) \ > >> > +type f_##type(type v) \ > >> > +{ \ > >> > + v[1] = 0; \ > >> > + return v; \ > >> > +} > >> > + > >> > +FOO(int8x8_t) > >> > +FOO(int16x4_t) > >> > +FOO(int32x2_t) > >> > + > >> > +FOO(int8x16_t) > >> > +FOO(int16x8_t) > >> > +FOO(int32x4_t) > >> > +FOO(int64x2_t) > >> > + > >> > +FOO(float16x4_t) > >> > +FOO(float32x2_t) > >> > + > >> > +FOO(float16x8_t) > >> > +FOO(float32x4_t) > >> > +FOO(float64x2_t) > >> > + > >> > +/* { dg-final { scan-assembler-times "ins\\tv\[0-9\]+\.b\\\[\[1\]\\\], wzr" 2 } } */ > >> > +/* { dg-final { scan-assembler-times "ins\\tv\[0-9\]+\.h\\\[\[1\]\\\], wzr" 4 } } */ > >> > +/* { dg-final { scan-assembler-times "ins\\tv\[0-9\]+\.s\\\[\[1\]\\\], wzr" 4 } } */ > >> > +/* { dg-final { scan-assembler-times "ins\\tv\[0-9\]+\.d\\\[\[1\]\\\], xzr" 2 } } */ > >> > >> Can you test big-endian too? I'd expect it to use different INS indices. > > Ah indeed, thanks for pointing out. > >> > >> It might be worth quoting the regexps with {...} rather than "...", > >> to reduce the number of backslashes needed. > > Does the attached patch look OK ? > > Yeah, OK for GCC 14, thanks. Thanks, committed after verifying bootstrap+test passes on aarch64-linux-gnu in: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=2c7bf8036dfe2f603f1c135dabf6415d8d28051b Thanks, Prathamesh > > Richard > > > > > Thanks, > > Prathamesh > >> > >> Thanks, > >> Richard > > > > [aarch64] Use wzr/xzr for assigning 0 to vector element. > > > > gcc/ChangeLog: > > * config/aaarch64/aarch64-simd.md (aarch64_simd_vec_set_zero): > > New pattern. > > > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/vec-set-zero.c: New test. > > > > diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md > > index 7f212bf37cd..7428e74beaf 100644 > > --- a/gcc/config/aarch64/aarch64-simd.md > > +++ b/gcc/config/aarch64/aarch64-simd.md > > @@ -1083,6 +1083,20 @@ > > [(set_attr "type" "neon_ins, neon_from_gp, neon_load1_one_lane")] > > ) > > > > +(define_insn "aarch64_simd_vec_set_zero" > > + [(set (match_operand:VALL_F16 0 "register_operand" "=w") > > + (vec_merge:VALL_F16 > > + (match_operand:VALL_F16 1 "aarch64_simd_imm_zero" "") > > + (match_operand:VALL_F16 3 "register_operand" "0") > > + (match_operand:SI 2 "immediate_operand" "i")))] > > + "TARGET_SIMD && exact_log2 (INTVAL (operands[2])) >= 0" > > + { > > + int elt = ENDIAN_LANE_N (, exact_log2 (INTVAL (operands[2]))); > > + operands[2] = GEN_INT ((HOST_WIDE_INT) 1 << elt); > > + return "ins\\t%0.[%p2], zr"; > > + } > > +) > > + > > (define_insn "@aarch64_simd_vec_copy_lane" > > [(set (match_operand:VALL_F16 0 "register_operand" "=w") > > (vec_merge:VALL_F16 > > diff --git a/gcc/testsuite/gcc.target/aarch64/vec-set-zero.c b/gcc/testsuite/gcc.target/aarch64/vec-set-zero.c > > new file mode 100644 > > index 00000000000..b34b902cf27 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/aarch64/vec-set-zero.c > > @@ -0,0 +1,40 @@ > > +/* { dg-do compile } */ > > +/* { dg-options "-O2" } */ > > + > > +#include "arm_neon.h" > > + > > +#define FOO(type) \ > > +type f_##type(type v) \ > > +{ \ > > + v[1] = 0; \ > > + return v; \ > > +} > > + > > +FOO(int8x8_t) > > +FOO(int16x4_t) > > +FOO(int32x2_t) > > + > > +FOO(int8x16_t) > > +FOO(int16x8_t) > > +FOO(int32x4_t) > > +FOO(int64x2_t) > > + > > +FOO(float16x4_t) > > +FOO(float32x2_t) > > + > > +FOO(float16x8_t) > > +FOO(float32x4_t) > > +FOO(float64x2_t) > > + > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.b\[1\], wzr} 2 { target aarch64_little_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.h\[1\], wzr} 4 { target aarch64_little_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.s\[1\], wzr} 4 { target aarch64_little_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.d\[1\], xzr} 2 { target aarch64_little_endian } } } */ > > + > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.b\[6\], wzr} 1 { target aarch64_big_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.b\[14\], wzr} 1 { target aarch64_big_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.h\[2\], wzr} 2 { target aarch64_big_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.h\[6\], wzr} 2 { target aarch64_big_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.s\[0\], wzr} 2 { target aarch64_big_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.s\[2\], wzr} 2 { target aarch64_big_endian } } } */ > > +/* { dg-final { scan-assembler-times {ins\tv[0-9]+\.d\[0\], xzr} 2 { target aarch64_big_endian } } } */