Artemiy Volkov <[email protected]> writes: > 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.
Agreed FWIW. I have a feeling that there was an existing example, but I can't find it now. Perhaps it was still while the SME work was internal (so that it looks from the submission like things were always designed the current way). I think the choice between "one mode" and "two modes" is simply: does the main operation in principle support mixed modes? If so, then the names of all patterns for that operation should have two modes. And one example of an instruction that has mixed modes is enough to establish that the associated operation can be mixed-mode. So going the other way: if we initially have single-mode patterns, and then add a mixed-mode pattern later, I think we should rename the existing single-mode patterns so that they have two modes. (I think the previous example that I mentioned was like this.) Richard
