[PATCH] D72799: [SVE] Add SVE2 patterns for unpredicated multiply instructions

[Danilo Carvalho Grael via Phabricator via llvm-commits]

dancgr marked 10 inline comments as done.
dancgr added a comment.

I'm not sure on some parts, but I have prepared a major update for this patch that I hope will fix most of @sdesmalen concerns.



================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1186
 
-def int_aarch64_sve_mul        : AdvSIMD_Pred2VectorArg_Intrinsic;
-def int_aarch64_sve_smulh      : AdvSIMD_Pred2VectorArg_Intrinsic;
-def int_aarch64_sve_umulh      : AdvSIMD_Pred2VectorArg_Intrinsic;
+def int_aarch64_sve_smulh      : AdvSIMD_2VectorArg_Intrinsic;
+def int_aarch64_sve_umulh      : AdvSIMD_2VectorArg_Intrinsic;
----------------
sdesmalen wrote:
> `smulh` needs to be predicated `AdvSIMD_Pred2VectorArg_Intrinsic`, so that we can implement the predicated ACLE intrinsic with this LLVM IR intrinsic.
> You can choose to optimize this predicated form (with ptrue predicate mask) into an unpredicated form using a ISel pattern.
Ok, but we have already a predicated smulh intrinsic, which is the int_aarch64_sve_smulh_m. Do we need the predicated version to be named exactly int_aarch64_sve_smulh?


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1192
+
+def int_aarch64_sve_mul_z      : AdvSIMD_Pred2VectorArg_Intrinsic;
+def int_aarch64_sve_smulh_z    : AdvSIMD_Pred2VectorArg_Intrinsic;
----------------
sdesmalen wrote:
> dancgr wrote:
> > efriedma wrote:
> > > This name seems strange.  Why "z"?
> > I just followed the name similar to the one for logical predicated instructions. I'm not sure if that is the correct way to name predicated intrinsics. I marked the place with a comment.
> Adding `_z` here implies that the false lanes will be zeroed, which isn't the case, as this is used with merging predication.
> These intrinsics are to implement the ACLE, which does the zeroing explicitly with a select. These intrinsics are merging, unless explicitly specified.
Ok, makes sense now.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:1555
 
 def int_aarch64_sve_and_z   : AdvSIMD_Pred2VectorArg_Intrinsic;
 def int_aarch64_sve_bic_z   : AdvSIMD_Pred2VectorArg_Intrinsic;
----------------
sdesmalen wrote:
> dancgr wrote:
> > efriedma wrote:
> > > dancgr wrote:
> > > > @efriedma Here!
> > > I think here the "_z" is meant to indicate that the predicate operand is "<Pg>/Z".
> > Ok, I will update it to _m to follow the same pattern.
> The reason that these intrinsics have _z in the name is because they implement an operation with zeroing predication (which maps directly to predicate AND that zeroes the false lanes).
> 
> Please don't change these suffixes, as this will give us trouble while we're in the process of implementing the ACLE upstream. We would need to update our Clang implementation to target intrinsics with slightly different names.
I'm not planning on changing these, I just wanted to understand the naming convention. Now it is clear to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72799