[PATCH] D100487: [AArch64][SVE] Lower MULHU/MULHS nodes to umulh/smulh instructions

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:92-93
   MUL_PRED,
+  SMULH_PRED,
+  UMULH_PRED,
   SDIV_PRED,
----------------
As the general rule we use the common node name suffixed with `_PRED`, so in this case these should be `MULHS_PRED` and `MULHU_PRED`.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:192
 def AArch64mul_p  : SDNode<"AArch64ISD::MUL_PRED",  SDT_AArch64Arith>;
+def AArch64smulh_p : SDNode<"AArch64ISD::SMULH_PRED", SDT_AArch64Arith>;
+def AArch64umulh_p : SDNode<"AArch64ISD::UMULH_PRED", SDT_AArch64Arith>;
----------------
Just a note, because of my other comment, to say that this is fine, here we typically prefer the AArch64 names as you've used here, although it'll be nicer still if you maintained the existing alphabetical ordering :)


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-int-mulh.ll:5
+; RUN: llc -aarch64-sve-vector-bits-min=1024 < %s | FileCheck %s -D#VBYTES=128 -check-prefixes=CHECK,VBITS_EQ_1024
+; RUN: llc -aarch64-sve-vector-bits-min=2048 < %s | FileCheck %s -D#VBYTES=256 -check-prefixes=CHECK,VBITS_EQ_1024
+
----------------
Should this be `VBITS_EQ_2048`?


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-int-mulh.ll:67
+; CHECK-LABEL: smulh_v32i8:
+; VBITS_EQ_256: ptrue [[PG:p[0-9]+]].b, vl[[#min(VBYTES,32)]]
+; VBITS_EQ_256-DAG: ld1b { [[OP1:z[0-9]+]].b }, [[PG]]/z, [x0]
----------------
Given you're using `EQ_256` I doubt the `#min` logic means anything.  That said, why not VBITS_GE_256, shouldn't the code be the same for larger vectors? 


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-int-mulh.ll:393
+; Don't use SVE for 64-bit vectors.
+define <1 x i64> @smulh_v1i64(<1 x i64> %op1, <1 x i64> %op2) #0 {
+; CHECK-LABEL: smulh_v1i64:
----------------
As there's no NEON instruction for i64 based vectors I'm wondering if it's worth using SVE for this case as well? much like we do for ISD::MUL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100487