c-rhodes marked an inline comment as done.
c-rhodes added inline comments.
================
Comment at: clang/include/clang/Basic/arm_sve.td:1115
+let ArchGuard = "defined(__ARM_FEATURE_SVE_BF16)" in {
+def SVREV_BF16 : SInst<"svrev[_{d}]", "dd", "b", MergeNone,
"aarch64_sve_rev">;
----------------
c-rhodes wrote:
> fpetrogalli wrote:
> > c-rhodes wrote:
> > > c-rhodes wrote:
> > > > fpetrogalli wrote:
> > > > > nit: could create a multiclass here like @sdesmalen have done in
> > > > > https://reviews.llvm.org/D82187, seems quite a nice way to keep the
> > > > > definition of the intrinsics together (look for `multiclass
> > > > > StructLoad`, for example)
> > > > it might be a bit tedious having separate multiclasses, what do you
> > > > think about:
> > > > ```multiclass SInstBF16<string n, string p, string t, MergeType mt,
> > > > string i = "",
> > > > list<FlagType> ft = [], list<ImmCheck> ch = []> {
> > > > def : SInst<n, p, t, mt, i, ft, ch>;
> > > > let ArchGuard = "defined(__ARM_FEATURE_SVE_BF16)" in {
> > > > def : SInst<n, p, "b", mt, i, ft, ch>;
> > > > }
> > > > }
> > > >
> > > > defm SVREV : SInstBF16<"svrev[_{d}]", "dd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_rev">;
> > > > defm SVSEL : SInstBF16<"svsel[_{d}]", "dPdd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_sel">;
> > > > defm SVSPLICE : SInstBF16<"svsplice[_{d}]", "dPdd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_splice">;
> > > > defm SVTRN1 : SInstBF16<"svtrn1[_{d}]", "ddd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_trn1">;
> > > > defm SVTRN2 : SInstBF16<"svtrn2[_{d}]", "ddd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_trn2">;
> > > > defm SVUZP1 : SInstBF16<"svuzp1[_{d}]", "ddd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_uzp1">;
> > > > defm SVUZP2 : SInstBF16<"svuzp2[_{d}]", "ddd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_uzp2">;
> > > > defm SVZIP1 : SInstBF16<"svzip1[_{d}]", "ddd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_zip1">;
> > > > defm SVZIP2 : SInstBF16<"svzip2[_{d}]", "ddd", "csilUcUsUiUlhfd",
> > > > MergeNone, "aarch64_sve_zip2">;```
> > > >
> > > > ?
> > > I've played around with this and it works great for instructions guarded
> > > on a single feature flag but falls apart for the .Q forms that also
> > > require `__ARM_FEATURE_SVE_MATMUL_FP64`. I suspect there's a nice way of
> > > handling it in tablegen by passing the features as a list of strings and
> > > joining them but I spent long enough trying to get that to work so I'm
> > > going to keep it simple for now.
> > > it might be a bit tedious having separate multiclasses, what do you think
> > > about:
> >
> > Sorry I think I misunderstood you when we last discussed this. I didn't
> > mean to write a multiclass that would work for ALL intrinsics that uses
> > regular types and bfloats.... I just meant to merge together those who were
> > using the same archguard and that you are adding in this patch.
> >
> > I think you could keep both macros in a single ArchGuard string:
> >
> > ```
> > multiclass SInstPerm<string n, string p, MergeType mt, string i> {
> > def : SInst<n, p, "csilUcUsUiUlhfd", mt, i>;
> > let ArchGuard = "defined(__ARM_FEATURE_SVE_BF16)" in {
> > def : SInst<n, p, "b", mt, i>;
> > }
> > }
> >
> > defm SVREV : SInstPerm<"svrev[_{d}]", "dd", MergeNone,
> > "aarch64_sve_rev">;
> > ...
> >
> > multiclass SInstPermMatmul<string n, string p, MergeType mt, string i> {
> > def : SInst<n, p, "csilUcUsUiUlhfd", mt, i>;
> > let ArchGuard = "defined(__ARM_FEATURE_SVE_BF16) &&
> > defined(__ARM_FEATURE_SVE_MATMUL_FP64)" in {
> > def : SInst<n, p, "b", mt, i>;
> > }
> > }
> >
> > def SVTRN1Q : SInstPermMatmul ...
> > ...
> > ```
> Sure, I understood you meant separate multiclasses for each intrinsic / group
> similar to what Sander implemented for structured loads / stores but I
> thought it would be quite abit of extra code to implement that, hence why I
> proposed a single multiclass that could handle this. I've experimented with
> the `SInstBF16` multiclass I mentioned above and have it working with an
> extra arg for arch features. I'll create a follow up patch and if people are
> happy with it we'll move forward with that, otherwise I'm happy to implement
> your suggestion.
> I'll create a follow up patch
https://reviews.llvm.org/D82450
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82182/new/
https://reviews.llvm.org/D82182
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
