[PATCH] D73576: [AArch64][SVE] Add SVE2 mla indexed intrinsics

[Danilo Carvalho Grael via Phabricator via llvm-commits]

dancgr added a comment.

I will make the changes suggested by Sander in a following patch, joined with the saturating multiply-add long intirnsics.



================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1088
+                 LLVMSubdivide2VectorType<0>,
+                 llvm_i64_ty],
+                [IntrNoMem, ImmArg<3>]>;
----------------
sdesmalen wrote:
> For consistency with other intrinsics, these need to use an `i32` value for the immediate. The reason is that the front-end will generate the calls to the intrinsics automatically and will use an `i32` for the immediates. Having one exception to the rule means this is something we'll need to fix in Clang or definition of the intrinsic later.
Sure, no problem. The reason I chose i64 was because the original InstrFormat class definition used VectorIndexH and VectorIndexS, which are i64 immediates.  I will make a switch to i32 formats.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1470
   // SVE2 integer multiply-add long (indexed)
-  defm SMLALB_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1000, "smlalb">;
-  defm SMLALT_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1001, "smlalt">;
-  defm UMLALB_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1010, "umlalb">;
-  defm UMLALT_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1011, "umlalt">;
-  defm SMLSLB_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1100, "smlslb">;
-  defm SMLSLT_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1101, "smlslt">;
-  defm UMLSLB_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1110, "umlslb">;
-  defm UMLSLT_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1111, "umlslt">;
+  defm SMLALB_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1000, "smlalb", int_aarch64_sve_smlalb>;
+  defm SMLALT_ZZZI : sve2_int_mla_long_by_indexed_elem<0b1001, "smlalt", int_aarch64_sve_smlalt>;
----------------
sdesmalen wrote:
> In order to be consistent with other intrinsics that have indexed form (such as `int_aarch64_sve_fmlalb_lane`), these indexed forms are better named `int_aarch64_sve_smlalb_lane`, where `int_aarch64_sve_smlalb` are used for the vectors unpredicated form.
> 
> It may come across as if I'm being a bit pedantic here with the naming, but not deviating unnecessarily from our downstream implementation (see D71712 for reference) really helps us when we go and upstream the Clang side of the ACLE (which we're preparing to upstream as we speak). We generate code for Clang to map C/C++ level builtins -> llvm ir intrinsics, and any changes to the intrinsic names like this, we will need to fix up in our mapping and tests. This probably isn't too complicated, but it would be another thing we'd need to fix, so better to avoid this if we can.
Sure, will update that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73576