[PATCH] D105283: [VP] Introducing VectorBuilder, the VP intrinsic builder

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

LGTM though we should probably add extra tests for `vp.select` and `vp.reduce.*` now that they're in.



================
Comment at: llvm/lib/IR/VectorBuilder.cpp:67
+  SmallVector<Value *, 6> IntrinParams;
+  size_t NumVPParams = InstOpArray.size() + (bool)MaskPosOpt + (bool)VLenPosOpt;
+  for (size_t ParamIdx = 0; ParamIdx < NumVPParams; ++ParamIdx) {
----------------
I'm thinking `.hasValue()` is a bit clearer than `(bool)`


================
Comment at: llvm/lib/IR/VectorBuilder.cpp:69
+  for (size_t ParamIdx = 0; ParamIdx < NumVPParams; ++ParamIdx) {
+    if (MaskPosOpt && (ParamIdx == (size_t)MaskPosOpt.getValue()))
+      IntrinParams.push_back(&requestMask());
----------------
don't need the parentheses around the equality comparison here or below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105283