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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11910-11913
+    case Intrinsic::aarch64_sve_ld1_gather:
+      return performLD1GatherCombine(N, DAG, AArch64ISD::GLD1);
+    case Intrinsic::aarch64_sve_ld1_gather_index:
+      return performLD1GatherCombine(N, DAG, AArch64ISD::GLD1_SCALED);
----------------
Nit: the style of the file seems to be more of  having a single invocation of the function shared by the two N->getOpcode(), with the ISD node selection inside the function.

```
static SDValue performLD1GatherCombine(SDNode *N, SelectionDAG &DAG,
                                       ) {
  unsigned Opcode;
  switch(N->getOpcode()) {
  default:
     llvm_unreachable();  // <- this would guarantee that the function is not invoked on something that it cannot handle yet?
  case case Intrinsic::aarch64_sve_ld1_gather:
     Opcode = AArch64ISD::GLD1;
     break;
   case ...
  }
  EVT RetVT = N->getValueType(0);
  assert(RetVT.isScalableVector() &&
         "Gather loads are only possible for SVE vectors");

}


//...
    case Intrinsic::aarch64_sve_ld1_gather:
    case Intrinsic::aarch64_sve_ld1_gather_index:
      return performLD1GatherCombine(N, DAG);
// ...
```


================
Comment at: llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h:654
+// <n x (M*P) x t> vector (such as index 1) are undefined.
+const unsigned SVEBitsPerBlock = 128;
+} // end namespace AArch64
----------------
`static constexpr unsigned ` should make sure that we don't run into duplicate variable declaration if the header get's included somewhere else (admittedly, an unlikely situation in this specific case).


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

https://reviews.llvm.org/D70542