[PATCH] D76929: [AArch64][SVE] Add SVE intrinsic for LD1RQ

[Andrzej Warzynski via Phabricator via cfe-commits]

andwar added a comment.

Cheers for working on this @kmclaughlin !



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1407
   case AArch64ISD::PTRUE:             return "AArch64ISD::PTRUE";
+  case AArch64ISD::LD1RQ:             return "AArch64ISD::LD1RQ";
   case AArch64ISD::LDNF1:             return "AArch64ISD::LDNF1";
----------------
[nit] In AArch64ISelLowering.h you added `LD1RQ` as the last load instruction, here it's the first load instruction.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11592
 
+static SDValue performLD1RQCombine(SDNode *N, SelectionDAG &DAG) {
+  SDLoc DL(N);
----------------
[Nit] I think that this method could be simplified and made more explicit:

```
static SDValue performLD1RQCombine(SDNode *N, SelectionDAG &DAG) {
  SDLoc DL(N);
  EVT VT = N->getValueType(0);

  EVT LoadVT = VT;
  if (VT.isFloatingPoint())
    LoadVT = VT.changeTypeToInteger();

  SDValue Ops[] = {N->getOperand(0), N->getOperand(2), N->getOperand(3)};
  SDValue Load = DAG.getNode(AArch64ISD::LD1RQ, DL, {LoadVT, MVT::Other}, Ops);

  if (VT.isFloatingPoint()) {
    SDValue LoadChain = SDValue(Load.getNode(), 1);
    Load = DAG.getMergeValues(
        {DAG.getNode(ISD::BITCAST, DL, VT, Load), LoadChain}, DL);
  }

  return Load;
}
```

This way:
 * there's only 1 `return` statement
 * you are being explicit about preserving the chain when generating the bit cast
 * `Load` replaces a bit non-descriptive `L`

This is a matter of style, so please use what feels best. 


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:495
+}]>;
+def SImmS16XForm : SDNodeXForm<imm, [{
+  return CurDAG->getTargetConstant(N->getSExtValue() / 16, SDLoc(N), MVT::i64);
----------------
IIUC, only this definition is need for `LD1RQ`. Could you add a note in the commit message to highlight that `SImmS2XForm`, `SImmS3XForm` and `SImmS4XForm` are added for consistency? Alternatively, only keep that one that's needed.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:16
 
 // Non-faulting loads - node definitions
 //
----------------
This comment should read `Non-faulting & first-faulting loads - node definitions`. That's my fault,, sorry!


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:23
 
+def SDT_AArch64_LD1RQ : SDTypeProfile<1, 2, [
+  SDTCisVec<0>, SDTCisVec<1>, SDTCisPtrTy<2>,
----------------
The (undocumented) style here is:

```
// <Load type 1> - node definitions
//
<Node definitions for type 1>

// <Load type 2> - node definitions
//
<Node definitions for type 2>
```

Could you move `LD1RQ` to a dedicated block and add a comment? Ta!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76929