On 05/08/2025 09:06, Kyrylo Tkachov wrote:
>
>
> > On 5 Aug 2025, at 11:00, Alex Coplan <alex.cop...@arm.com> wrote:
> >
> > Hi Kyrill,
> >
> > Sorry for the slow reply, I was away on holiday until yesterday.
> >
> > On 15/07/2025 13:08, Kyrylo Tkachov wrote:
> >> Hi Alex,
> >>
> >>> On 15 Jul 2025, at 14:59, Alex Coplan <alex.cop...@arm.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> The predication of the SVE2 FP8 dot product insns was relying on the
> >>> architectural dependency:
> >>>
> >>> FEAT_FP8DOT2 => FEAT_FP8DOT4
> >>>
> >>> which was relaxed in GCC as of
> >>> r15-7480-g299a8e2dc667e795991bc439d2cad5ea5bd379e2, thus leading to
> >>> unrecognisable insn ICEs when compiling a two-way FDOT with just
> >>> +fp8dot2. This patch fixes the predication of the insns to test for
> >>> the correct feature bit depending on the value of the mode iterator.
> >>>
> >>> Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk and backport
> >>> to GCC 15?
> >>>
> >>> Thanks,
> >>> Alex
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> PR target/120986
> >>> * config/aarch64/aarch64-sve2.md (@aarch64_sve_dot<mode>):
> >>> Adjust insn predicate to use new mode attribute which checks
> >>> for the correct feature bit depending on the mode.
> >>> (@aarch64_sve_dot_lane<mode>): Likewise.
> >>> * config/aarch64/iterators.md (HAVE_FP8_DOT_INSN): New.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>> PR target/120986
> >>> * gcc.target/aarch64/torture/pr120986-1.c: New test.
> >>> ---
> >>> gcc/config/aarch64/aarch64-sve2.md | 4 ++--
> >>> gcc/config/aarch64/iterators.md | 4 ++++
> >>> gcc/testsuite/gcc.target/aarch64/torture/pr120986-1.c | 10 ++++++++++
> >>> 3 files changed, 16 insertions(+), 2 deletions(-)
> >>> create mode 100644 gcc/testsuite/gcc.target/aarch64/torture/pr120986-1.c
> >>>
> >>> <0001-aarch64-Fix-predication-of-FP8-FDOT-insns-PR120986.patch>
> >>
> >> +(define_mode_attr HAVE_FP8_DOT_INSN [(VNx4SF "TARGET_SSVE_FP8DOT4")
> >> + (VNx8HF "TARGET_SSVE_FP8DOT2")])
> >> +
> >> +
> >>
> >> I think a more natural way would be to have a new mode iterator with the
> >> same contents as SVE_FULL_HSF, say SVE_FULL_HSF_FP8 but have the TARGET_*
> >> conditions guarding those modes.
> >> For example:
> >> (define_mode_iterator SVE_FULL_HSF_FP8 [(VNx4SF "TARGET_SSVE_FP8DOT4")
> >> (VNx8HF “TARGET_SSVE_FP8DOT2")])
> >
> > Thanks for the suggestion. I agree this way seems more natural, for some
> > reason
> > it hadn't occurred to me at the time to restrict the availability of the
> > modes
> > themselves.
> >
> > Re: naming, would you be OK with SVE_FULL_HSF_FP8_FDOT for this
> > iterator? Normally it's a good idea to keep iterator names as general
> > as possible, but the conditions on the modes make this iterator quite
> > specific to the FP8 FDOT instructions. Just wanted to check before pushing.
> >
>
> Yes that’s fine, I don’t feel strongly about it.
Ack, I've now pushed these two after re-testing as:
80b0e4ad2f6 aarch64: Relax fpm_t assert to allow const_ints [PR120986]
75eabf69d80 aarch64: Fix predication of FP8 FDOT insns [PR120986]
Thanks,
Alex
> Thanks,
> Kyrill
>
> > Thanks,
> > Alex
> >
> >>
> >> diff --git a/gcc/testsuite/gcc.target/aarch64/torture/pr120986-1.c
> >> b/gcc/testsuite/gcc.target/aarch64/torture/pr120986-1.c
> >> new file mode 100644
> >> index 00000000000..8777f1b7711
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/aarch64/torture/pr120986-1.c
> >> @@ -0,0 +1,10 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-march=armv8.2-a+sve2+fp8dot2" } */
> >>
> >> Very minor but this bug is about arch gating rather than something that’s
> >> hit with optimization, I don’t think there’s value in putting it in the
> >> torture directory. I’d put it under normal gcc.target/aarch64.
> >>
> >> Ok with these changes if you agree.
> >> Thanks,
> >> Kyrill
>
>
