[PATCH] D105871: Followup to D99355: implementation of sdag support for vp load/store/gather/scatter.

[Simon Moll via Phabricator via llvm-commits]

simoll added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:2347
+
+  // VPLoadSDNode (Chain, ptr, offset, mask, VLen)
+  // VPStoreSDNode (Chain, data, ptr, offset, mask, VLen)
----------------
frasercrmck wrote:
> nit: inconsistent capitalization, plus `VLen` is maybe better known at `EVL`.
> 
> Also it doesn't have to be `i32`. It may be widened to a target-specific value, e.g.,
> ```
>   MVT EVLParamVT = TLI.getVPExplicitVectorLengthTy();
> ```
> Not sure how best to describe that in the comments.
> Not sure how best to describe that in the comments.

`// The type of EVL is TLI.getVPExplicitVectorLengthTy()` ?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7596
+    ISD::MemIndexedMode AM, ISD::LoadExtType ExtType, EVT VT, const SDLoc &dl,
+    SDValue Chain, SDValue Ptr, SDValue Offset, SDValue Mask, SDValue VLen,
+    MachinePointerInfo PtrInfo, EVT MemVT, Align Alignment,
----------------
frasercrmck wrote:
> I think all of these `VLen` params may be better as `EVL` since that's how they're known in the rest of the code. Any thoughts, @simoll?
+1 Let's stick to `EVL` for consistency.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105871