On Tue, Sep 22, 2026 at 10:06:09AM +0100, Ezra Sitorus wrote:
> > Hi Ezra,
>
> Hi!
>
> > Not an official reviewer, but some comments below:
>
> Thanks a lot for taking a look. I just have one question.
>
> > > --- a/gcc/config/aarch64/aarch64-sve-builtins-base.cc
> > > +++ b/gcc/config/aarch64/aarch64-sve-builtins-base.cc
> > > @@ -2352,7 +2352,13 @@ public:
> > > else
> > > {
> > > if (e.type_suffix_ids[1] == NUM_TYPE_SUFFIXES)
> > > - icode = code_for_aarch64_sve (UNSPEC_FMMLA, e.vector_mode (0));
> > > + {
> > > + if (e.type_suffix (0).element_bytes == 2)
> > > + icode = code_for_aarch64_sve2_fmmla (e.vector_mode (0),
> > > + e.vector_mode (0));
> > > + else
> > > + icode = code_for_aarch64_sve (UNSPEC_FMMLA, e.vector_mode (0));
> > > + }
> >
> > I think the current implementation of svmmla_impl::expand () should
> > already be able to handle this, as long as your RTL patterns are defined
> > ? The call to
> >
> > icode = code_for_aarch64_sve2 (UNSPEC_FMMLA,
> > e.vector_mode (0),
> > e.vector_mode (1));
> >
> > requires that the pattern name uses one iterator for the unspec, followed
> > by two for operand modes? Something like ...
> >
> > ... "@aarch64_sve2_<sve_fp_op><mode><mode>" ?
> >
> > > + [(set (match_operand:SVE_FULL_HF_FMMLA 0 "register_operand")
> > > + (unspec:SVE_FULL_HF_FMMLA
> > > + [(match_operand:SVE_FULL_HF_FMMLA 2 "register_operand")
> > > + (match_operand:SVE_FULL_HF_FMMLA 3 "register_operand")
> > > + (match_operand:SVE_FULL_HF_FMMLA 1 "register_operand")]
> > > + FMMLA))]
> > > + "TARGET_NON_STREAMING && (<is_bf16> ? TARGET_SVE_B16MM :
> > > TARGET_SVE2p2)"
> > > + {@ [ cons: =0 , 1 , 2 , 3 ]
> > > + [ w , 0 , w , w ] <b>fmmla\t%0.h, %2.h, %3.h
> > > + }
> > > + [(set_attr "sve_type" "sve_fp_mul")]
> > > +)
>
> With this part, could I just have the mode appear once like this:
>
> (define_insn "@aarch64_sve_<sve_fp_op><mode>"
>
> since it's non-widening? Something similar appears in aarch64-sve.md:
>
> (define_insn "@aarch64_sve_<sve_fp_op><mode>"
> [(set (match_operand:SVE_MATMULF 0 "register_operand")
> (unspec:SVE_MATMULF
> ...
> so I don't think there's some convention to have it twice? I'll put it in my
> next revision.
The reason I'm suggesting to use <mode><mode> here is that it will make
the auto-generated code_for_ function look like:
code_for_aarch64_sve2 (int unspec, mode m1, mode m2)
rather than
code_for_aarch64_sve2 (int unspec, mode m1),
where each operand matches a successive iterator in the pattern name (see
also [0]).
The first signature matches the already existing ones for widening SVE2
FMMLA. This means that you can afford not to change any code in
svmmla_impl::expand () and pass the complexity to the RTL level.
By contrast, if you had different signatures for widening and non-widening
FMMLA, you'd have to duplicate the call to code_for_aarch64_sve2 ()
depending on the incoming vector modes. Something like:
if (e.vector_mode (0) == e.vector_mode (1)) // non-widening
icode = code_for_aarch64_sve2 (UNSPEC_FMMLA,
e.vector_mode (0));
else // widening
icode = code_for_aarch64_sve2 (UNSPEC_FMMLA,
e.vector_mode (0),
e.vector_mode (1));
I can't easily find a precedent of adding non-widening versions of an op
after having widening ones for a while, but personally I'd lean towards
keeping the C++ code simple if possible.
Hope this helps,
Artemiy
[0] https://gcc.gnu.org/onlinedocs/gccint/Parameterized-Names.html
>
> Thanks again for the reviews!
> Ezra
