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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

@dancgr I see you already committed the patch, but could you please still address my comments?



================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1088
+                 LLVMSubdivide2VectorType<0>,
+                 llvm_i64_ty],
+                [IntrNoMem, ImmArg<3>]>;
----------------
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.


================
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>;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73576