[PATCH] D70542: [AArch64][SVE] Add intrinsics for gather loads with 64-bit offsets

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for this patch @andwar! I think the patch could do with some cleanup, but most of it looks quite straightforward.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:617
   setTargetDAGCombine(ISD::SIGN_EXTEND);
+  setTargetDAGCombine(ISD::SIGN_EXTEND_INREG);
   setTargetDAGCombine(ISD::BITCAST);
----------------
Is this change supposed to be here?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:776
     // directly.
-    for (MVT VT : MVT::fixedlen_vector_valuetypes()) {
+    for (MVT VT : MVT::vector_valuetypes()) {
       setOperationAction(ISD::SIGN_EXTEND_INREG, VT, Expand);
----------------
is this change supposed to be here?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:835
   PredictableSelectIsExpensive = Subtarget->predictableSelectIsExpensive();
+
 }
----------------
nit: unrelated change.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:2982
 
+
 SDValue AArch64TargetLowering::LowerOperation(SDValue Op,
----------------
nit: unrelated change.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11783
+  EVT HwRetVt = RetVT;
+  switch (RetVT.getVectorNumElements()) {
+  default:
----------------
Can we move this functionality into a separate function? e.g. something like `MVT getPackedIntegerSVEType(MVT EltTy)`.
That should simplify this function quite a bit.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11821
+  // means that we can avoid adding TableGen patterns for FPs.
+  if (RetVT.isFloatingPoint()) {
+    EVT OutFpVT = RetVT;
----------------
Can this code not simply do a BITCAST to `OutVT`?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:228
   STZ2G
-
 };
----------------
nit: unrelated change.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:26
+
+def SVEAddrModeRegReg8  : ComplexPattern<i64, 2, "SelectSVERegRegAddrMode<0>", []>;
+
----------------
SVEAddrModeRegReg8 is not used anywhere, please remove.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1154
 
+  /* multiclass ldff1_gather<Instruction I, ValueType Ty, SDPatternOperator Load, ValueType PredTy, ComplexPattern AddrCP, ValueType MemVT> { */
+  /*     // base + index */
----------------
andwar wrote:
> I missed this, sorry! I will remove it in the next patch.
please remove.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:5241
 
-
 class sve_mem_32b_gld_vi<bits<4> opc, string asm, Operand imm_ty>
----------------
nit: unrelated change.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-gather-loads-64bit-offset.ll:11
+; CHECK: ld1b { z0.d }, p0/z, [x0, z0.d]
+; CHECK-NEXT:	mov	w8, #255
+; CHECK-NEXT: mov	z1.d, x8
----------------
The check-lines for the sign/zero extend are not really necessary, but it will make it more obvious to see them removed in a patch that folds the sign/zero extension into the load itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70542