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

[Simon Moll via Phabricator via llvm-commits]

simoll added a comment.

`vp.merge` and `vp.select` do not ignore masked-off lanes. We probably shouldn't treat their mask parameters as "masks" in the VP sense. I suggest we stop reporting their MaskPos in `VPintrinsics.def`.



================
Comment at: llvm/include/llvm/IR/VectorBuilder.h:83
+  VectorBuilder &setStaticVL(unsigned NewFixedVL) {
+    StaticVectorLength = ElementCount::getFixed(NewFixedVL);
+    return *this;
----------------
craig.topper wrote:
> How do you set StaticVectorLength to a scalable value?
Scalable types aren't supported in this first patch. Planned for a followup.


================
Comment at: llvm/lib/IR/VectorBuilder.cpp:75
+    if (ParamIdx < InstOpArray.size())
+      IntrinParams.push_back(InstOpArray[ParamIdx]);
+  }
----------------
craig.topper wrote:
> Does this assume the Mask and EVL are at the end? If they aren't then ParamIdx isn't the correct index here if the Mask and EVL were earlier. Assuming there aren't empty slots for them in InstOpArray.
> 
> If they are always at the end and we're going to assume, that then there's not a lot of point for handling them inside the loop. You can just copy the InstOps, then add mask and evl if they exist.
There is an issue with `ParamIdx` at the moment. If the loop is past the first mask or evl parameter the `ParamIdx` will be off by one for the other one. It is okay for the other parameters though (note that there is no `else` in front of the last `if`).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105283