On Thu, Sep 03, 2026 at 01:09:46PM +0100, Tamar Christina wrote:
> Hi Artemiy,
>
> > -----Original Message-----
> > From: Artemiy Volkov <[email protected]>
> > Sent: 27 August 2026 15:25
> > To: [email protected]
> > Cc: [email protected]; Richard Earnshaw <[email protected]>;
> > Tamar Christina <[email protected]>;
> > [email protected]; Artemiy Volkov
> > <[email protected]>
> > Subject: [PATCH] aarch64: fix combinez patterns for FP16 types [PR126983]
> >
> > The combinez patterns for scalar and partial vector operands added in
> > r17-898-g920eeb67a3537b use the FMOV instruction indiscriminately,
> > including for scalar FP16 modes in absence of the fp16 extension, causing
> > the assemble-time failure reported in PR126983. Fix this by specializing
> > the
> > patterns for HF and BF modes and using the indexed INS instruction as
> > fallback.
> >
> > This patch also adds the new vec-init-23-nofp16.c test, which simply tries
> > to assemble the testcases from vec-init-23.c without the fp16 extension
> > (including test_float16_6 that triggers the problem), as well as adjusts
> > expected codegen for one testcase in sve/vec_init_5.c.
> >
> > Bootstrapped and regtested on aarch64-linux-gnu.
> >
> > gcc/ChangeLog:
> >
> > * config/aarch64/aarch64-simd.md
> > (*aarch64_combine_internal<mode>): Adjust to use the new
> > VS32_I_VSSUB64_F iterator.
> > (*aarch64_combine_internal_be<mode>): Likewise.
> > (*aarch64_combinez<mode>): Split into FP16 and non-FP16 patterns.
> > (*aarch64_combinez_be<mode): Likewise.
> > * config/aarch64/iterators.md: (VSSUB64_F): Remove iterator.
> > (VS32_I_SUB64_F): Likewise.
> > (VSUB64_F): New iterator.
> > (VS32_I_VSUB64_F): Likewise.
> > (VS32_I_VSSUB64_F): Likewise.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/aarch64/sve/vec_init_5.c: Adjust testcase.
> > * gcc.target/aarch64/vec-init-23-nofp16.c: New test.
> > ---
> > gcc/config/aarch64/aarch64-simd.md | 49 ++++++++++++++++---
> > gcc/config/aarch64/iterators.md | 9 ++--
> > .../gcc.target/aarch64/sve/vec_init_5.c | 6 +--
> > .../gcc.target/aarch64/vec-init-23-nofp16.c | 4 ++
> > 4 files changed, 54 insertions(+), 14 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.target/aarch64/vec-init-23-nofp16.c
> >
> > diff --git a/gcc/config/aarch64/aarch64-simd.md
> > b/gcc/config/aarch64/aarch64-simd.md
> > index f9b2fd6b2db..98f5ffb54b8 100644
> > --- a/gcc/config/aarch64/aarch64-simd.md
> > +++ b/gcc/config/aarch64/aarch64-simd.md
> > @@ -5003,8 +5003,8 @@
> > (define_insn "*aarch64_combine_internal<mode>"
> > [(set (match_operand:<VDBL> 0 "register_operand")
> > (vec_concat:<VDBL>
> > - (match_operand:VS32_I_SUB64_F 1 "register_operand")
> > - (match_operand:VS32_I_SUB64_F 2
> > "aarch64_simd_nonimmediate_operand")))]
> > + (match_operand:VS32_I_VSSUB64_F 1 "register_operand")
> > + (match_operand:VS32_I_VSSUB64_F 2
> > "aarch64_simd_nonimmediate_operand")))]
> > "TARGET_FLOAT
> > && !BYTES_BIG_ENDIAN"
> > {@ [ cons: =0 , 1 , 2 ; attrs: type , arch ]
> > @@ -5050,8 +5050,8 @@
> > (define_insn "*aarch64_combine_internal_be<mode>"
> > [(set (match_operand:<VDBL> 0 "register_operand")
> > (vec_concat:<VDBL>
> > - (match_operand:VS32_I_SUB64_F 2
> > "aarch64_simd_nonimmediate_operand")
> > - (match_operand:VS32_I_SUB64_F 1 "register_operand")))]
> > + (match_operand:VS32_I_VSSUB64_F 2
> > "aarch64_simd_nonimmediate_operand")
> > + (match_operand:VS32_I_VSSUB64_F 1 "register_operand")))]
> > "TARGET_FLOAT
> > && BYTES_BIG_ENDIAN"
> > {@ [ cons: =0 , 1 , 2 ; attrs: type , arch ]
> > @@ -5107,8 +5107,8 @@
> > (define_insn "*aarch64_combinez<mode>"
> > [(set (match_operand:<VDBL> 0 "register_operand")
> > (vec_concat:<VDBL>
> > - (match_operand:VS32_I_SUB64_F 1 "nonimmediate_operand")
> > - (match_operand:VS32_I_SUB64_F 2
> > "aarch64_simd_or_scalar_imm_zero")))]
> > + (match_operand:VS32_I_VSUB64_F 1 "nonimmediate_operand")
> > + (match_operand:VS32_I_VSUB64_F 2
> > "aarch64_simd_or_scalar_imm_zero")))]
> > "TARGET_FLOAT && !BYTES_BIG_ENDIAN"
> > {@ [ cons: =0 , 1 ; attrs: type ]
> > [ w , w ; neon_move ] fmov\t%<single_type>0,
> > %<single_type>1
> > @@ -5119,6 +5119,22 @@
> > }
> > )
> >
> > +(define_insn "*aarch64_combinez<mode>"
> > + [(set (match_operand:<VDBL> 0 "register_operand")
> > + (vec_concat:<VDBL>
> > + (match_operand:HFBF 1 "nonimmediate_operand")
> > + (match_operand:HFBF 2 "aarch64_simd_or_scalar_imm_zero")))]
> > + "TARGET_FLOAT && !BYTES_BIG_ENDIAN"
> > + {@ [ cons: =0 , 1 ; attrs: type, arch ]
> > + [ w , w ; neon_move , fp16 ] fmov\t%<single_type>0,
> > %<single_type>1
> > + [ w , r ; neon_from_gp , fp16 ] fmov\t%<single_type>0,
> > %<single_wx>1
> > + [ w , 0 ; neon_move , simd ] ins\t%0.<single_type>[1],
> > wzr
> > + [ w , m ; neon_load1_1reg , simd ] ldr\t%<single_type>0, %1
> > + [ r , r ; mov_reg , * ] uxtw\t%x0, %w1
> > + [ r , m ; load_4 , * ] ldr<size>\t%<single_wx>0,
> > %1
> > + }
> > +)
Hi Tamar, thanks for taking a look,
>
> These new patterns look a bit broad though since for TARGET_FLOAT &&
> !TARGET_SIMD_F16INST
> you now don't have w <- w or w <- r alternatives and would likely cause a
> reload since the predicate
> becomes wider than the constraints.
>
> I think it would be better to keep the original iterators and patterns but
> add constraints to them
>
> (define_mode_iterator VSSUB64_F [V2HF V2BF
> (HF "TARGET_SIMD_F16INST")
> (BF "TARGET_SIMD_F16INST")])
> (define_mode_iterator VS32_I_SUB64_F [V4QI V2HI VSSUB64_F])
>
> And then just provide new patterns for the fallbacks
>
> (define_insn "*aarch64_combine_internal<mode>"
> [(set (match_operand:<VDBL> 0 "register_operand")
> (vec_concat:<VDBL>
> (match_operand:HFBF 1 "register_operand")
> (match_operand:HFBF 2 "aarch64_simd_nonimmediate_operand")))]
> "TARGET_FLOAT && TARGET_SIMD && !TARGET_SIMD_F16INST && !BYTES_BIG_ENDIAN"
> {@ [ cons: =0 , 1 , 2 ; attrs: type , arch ]
> [ w , w , w ; neon_permute , simd ]
> zip1\t%0.<Vdduptype>, %1.<Vdduptype>, %2.<Vdduptype>
> [ w , 0 , w ; neon_move , simd ]
> mov\t%0.<single_type>[1], %2.<single_type>[0]
> [ w , 0 , Utv ; neon_load1_one_lane , simd ]
> ld1\t{%0.<single_type>}[1], %2
> [ w , 0 , r ; neon_from_gp , simd ]
> ins\t%0.<single_type>[1], %<single_wx>2
> }
> )
>
> And similar for aarch64_combinez<mode>
I understand your logic, but based on the HW instructions used by the
patterns, there is no need to change aarch64_combine_internal since it
doesn't use FMOV anywhere and so will work for simd && !fp16. Or do you
think it is worth doing for symmetry with aarch64_combinez?
Changing only aarch64_combinez as I'm proposing means that we need to add
some new iterators; I could do that the way you suggested, adding something
like:
(define_mode_iterator VSSUB64_F_NO_FP16 [V2HF V2BF
(HF "TARGET_SIMD_F16INST")
(BF "TARGET_SIMD_F16INST")])
and changing aarch64_combinez accordingly. What do you think?
>
> > +
> > (define_insn "*aarch64_combinez_be<mode>"
> > [(set (match_operand:<VDBL> 0 "register_operand")
> > (vec_concat:<VDBL>
> > @@ -5147,8 +5163,8 @@
> > (define_insn "*aarch64_combinez_be<mode>"
> > [(set (match_operand:<VDBL> 0 "register_operand")
> > (vec_concat:<VDBL>
> > - (match_operand:VS32_I_SUB64_F 2
> > "aarch64_simd_or_scalar_imm_zero")
> > - (match_operand:VS32_I_SUB64_F 1 "nonimmediate_operand")))]
> > + (match_operand:VS32_I_VSUB64_F 2
> > "aarch64_simd_or_scalar_imm_zero")
> > + (match_operand:VS32_I_VSUB64_F 1 "nonimmediate_operand")))]
> > "TARGET_FLOAT && BYTES_BIG_ENDIAN"
> > {@ [ cons: =0 , 1 ; attrs: type ]
> > [ w , w ; neon_move ] fmov\t%<single_type>0,
> > %<single_type>1
> > @@ -5159,6 +5175,23 @@
> > }
> > )
> >
> > +(define_insn "*aarch64_combinez_be<mode>"
> > + [(set (match_operand:<VDBL> 0 "register_operand")
> > + (vec_concat:<VDBL>
> > + (match_operand:HFBF 2 "aarch64_simd_or_scalar_imm_zero")
> > + (match_operand:HFBF 1 "nonimmediate_operand")))]
> > + "TARGET_FLOAT && BYTES_BIG_ENDIAN"
> > + {@ [ cons: =0 , 1 ; attrs: type, arch ]
> > + [ w , w ; neon_move , fp16 ] fmov\t%<single_type>0,
> > %<single_type>1
> > + [ w , r ; neon_from_gp , fp16 ] fmov\t%<single_type>0,
> > %<single_wx>1
> > + [ w , 0 ; neon_move , simd ] ins\t%0.<single_type>[1],
> > wzr
> > + [ w , m ; neon_load1_1reg , simd ] ldr\t%<single_type>0, %1
> > + [ r , r ; mov_reg , * ] uxtw\t%x0, %w1
> > + [ r , m ; load_4 , * ] ldr<size>\t%<single_wx>0,
> > %1
>
> Also are these right? HFBF -> <VDBL> means creating a 32-bit vector from
> 16-bit data.
>
> uxtw would zero extend from 32-bit to 64 but your input vector elements
> should be 16-bits.
> So I think you want uxth and ldrh here since you're zeroing bit 16+.
Yes of course, that should be uxt<size> in the (r,r) alternatives throughout.
Thanks,
Artemiy
>
> Thanks,
> Tamar
>
> > + }
> > +)
> > +
> > +
> > ;; Form a vector whose first half (in array order) comes from operand 1
> > ;; and whose second half (in array order) comes from operand 2.
> > ;; This operand order follows the RTL vec_concat operation.
> > diff --git a/gcc/config/aarch64/iterators.md
> > b/gcc/config/aarch64/iterators.md
> > index b0571460ec7..296faf6bb2e 100644
> > --- a/gcc/config/aarch64/iterators.md
> > +++ b/gcc/config/aarch64/iterators.md
> > @@ -253,11 +253,14 @@
> > ;; All sub-32-bit integer modes.
> > (define_mode_iterator VSSUB32_I [V2QI QI HI])
> >
> > -;; All sub-64-bit floating-point modes.
> > -(define_mode_iterator VSSUB64_F [V2HF V2BF HF BF])
> > +;; All sub-64-bit vector floating-point modes.
> > +(define_mode_iterator VSUB64_F [V2HF V2BF])
> > +
> > +;; All 32-bit integer and vector sub-64-bit floating point modes.
> > +(define_mode_iterator VS32_I_VSUB64_F [V4QI V2HI VSUB64_F])
> >
> > ;; All 32-bit integer and sub-64-bit floating point modes.
> > -(define_mode_iterator VS32_I_SUB64_F [V4QI V2HI VSSUB64_F])
> > +(define_mode_iterator VS32_I_VSSUB64_F [V4QI V2HI VSUB64_F HFBF])
> > ;; All Advanced SIMD modes suitable for moving, loading, and storing.
> > (define_mode_iterator VALL_F16 [V8QI V16QI V4HI V8HI V2SI V4SI V2DI
> > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/vec_init_5.c
> > b/gcc/testsuite/gcc.target/aarch64/sve/vec_init_5.c
> > index 0dd085a9423..d02dfadadb5 100644
> > --- a/gcc/testsuite/gcc.target/aarch64/sve/vec_init_5.c
> > +++ b/gcc/testsuite/gcc.target/aarch64/sve/vec_init_5.c
> > @@ -217,12 +217,12 @@
> >
> > /*
> > ** test_float16_6:
> > -** fcvt h2, s0
> > +** fcvt h0, s0
> > ** fcvt h1, s1
> > ** fmov h31, 1.0e\+0
> > -** fmov h2, h2
> > +** fmov h0, h0
> > ** zip1 v1\.4h, v1\.4h, v31\.4h
> > -** dup v0\.2s, v2\.s\[0\]
> > +** dup v0\.2s, v0\.s\[0\]
> > ** dup v1\.2s, v1\.s\[0\]
> > ** zip1 v0\.8h, v0\.8h, v1\.8h
> > ** dup z0\.q, z0\.q\[0\]
> > diff --git a/gcc/testsuite/gcc.target/aarch64/vec-init-23-nofp16.c
> > b/gcc/testsuite/gcc.target/aarch64/vec-init-23-nofp16.c
> > new file mode 100644
> > index 00000000000..636f4758c19
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/vec-init-23-nofp16.c
> > @@ -0,0 +1,4 @@
> > +/* { dg-do assemble } */
> > +/* { dg-options "-O2 -march=armv8-a" } */
> > +
> > +#include "vec-init-23.c"
> > --
> > 2.43.0
>
