Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> writes: > On Wed, 18 Jan 2023 at 19:59, Richard Sandiford > <richard.sandif...@arm.com> wrote: >> >> Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> writes: >> > On Tue, 17 Jan 2023 at 18:29, Richard Sandiford >> > <richard.sandif...@arm.com> wrote: >> >> >> >> Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> 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<mode>" >> >> > [(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 (<nunits>, exact_log2 (INTVAL >> >> > (operands[2]))); >> >> > operands[2] = GEN_INT ((HOST_WIDE_INT) 1 << elt); >> >> > return "ins\\t%0.<Vetype>[%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 <vwcore>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<mode>): >> > 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<q>, neon_from_gp<q>, >> > neon_load1_one_lane<q>")] >> > ) >> > >> > +(define_insn "aarch64_simd_vec_set_zero<mode>" >> > + [(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 (<nunits>, exact_log2 (INTVAL (operands[2]))); >> > + operands[2] = GEN_INT ((HOST_WIDE_INT) 1 << elt); >> > + return "ins\\t%0.<Vetype>[%p2], <vwcore>zr"; >> > + } >> > +) >> > + >> > (define_insn "@aarch64_simd_vec_copy_lane<mode>" >> > [(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<mode>): > 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<q>, neon_from_gp<q>, neon_load1_one_lane<q>")] > ) > > +(define_insn "aarch64_simd_vec_set_zero<mode>" > + [(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 (<nunits>, exact_log2 (INTVAL (operands[2]))); > + operands[2] = GEN_INT ((HOST_WIDE_INT) 1 << elt); > + return "ins\\t%0.<Vetype>[%p2], <vwcore>zr"; > + } > +) > + > (define_insn "@aarch64_simd_vec_copy_lane<mode>" > [(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. It might be worth quoting the regexps with {...} rather than "...", to reduce the number of backslashes needed. Thanks, Richard