[PATCH] D82182: [AArch64][SVE] Add bfloat16 support to perm and select intrinsics

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli 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:
> 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 ...
...
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82182/new/

https://reviews.llvm.org/D82182