[PATCH] D109584: [VP] Implementing expansion pass for VP load and store.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:166
 
+  /// \brief Lower this VP memory operation to an unpredicated intrinsic.
+  Value *expandPredicationInMemoryIntrinsic(IRBuilder<> &Builder,
----------------
`unpredicated` seems misleading here, since we're using `masked.load` and `masked.store`: those are predicated in a sense.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:406
+    llvm_unreachable("Not a VP memory intrinsic");
+  case Intrinsic::vp_store: {
+    if (IsUnmasked) {
----------------
nit, but I don't think you need these braces around the switch statements. `NewStore`/`NewLoad` are defined in their own scope.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:408
+    if (IsUnmasked) {
+      StoreInst *NewStore = Builder.CreateStore(DataParam, PtrParam, false);
+      if (AlignOpt.hasValue())
----------------
I'd prefer something like `/*IsVolatile*/ false`


================
Comment at: llvm/test/CodeGen/Generic/expand-vp-load-store.ll:15
+;
+  %load = call <vscale x 1 x i64> @llvm.vp.load.nxv1i64.p0nxv1i64(<vscale x 1 x i64>* %ptr, <vscale x 1 x i1> %m, i32 %evl)
+  ret <vscale x 1 x i64> %load
----------------
We should be testing the `IsUnmasked` path here too.


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

https://reviews.llvm.org/D109584