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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits