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

[Simon Moll via Phabricator via llvm-commits]

simoll marked an inline comment as done.
simoll added inline comments.


================
Comment at: llvm/lib/IR/VectorBuilder.cpp:80
+
+    bool maskBeforeVLen =
+        MaskPosOpt.getValueOr(0) <= VLenPosOpt.getValueOr(NumVPParams);
----------------
craig.topper wrote:
> What if we just called `IntrinParams.resize(NumVPParams);` and then assigned the mask and vl into their slots?
I like it


================
Comment at: llvm/unittests/IR/VectorBuilderTest.cpp:130
+
+static bool isLegalConstEVL(Value *Val, unsigned ExpectedEVL) {
+  if (!isa<ConstantInt>(Val))
----------------
craig.topper wrote:
> Can simplify a little by merging the isa and cast to a single dyn_cast.
> 
> ```
> static bool isLegalConstEVL(Value *Val, unsigned ExpectedEVL) {
>   auto *ConstEVL = dyn_cast<ConstantInt>(Val);
>   if (!ConstEVL)
>     return false;
> 
>   // Value check.
>   
>   if (ConstEVL->getZExtValue() != ExpectedEVL)
>     return false;
> 
>   // Type check.
>   return ConstEVL->getType()->isIntegerTy(32);
> }
> ```
> 
> Or go really packed and do.
> ```
> static bool isLegalConstEVL(Value *Val, unsigned ExpectedEVL) {
>   auto *ConstEVL = dyn_cast<ConstantInt>(Val);
>   return ConstEVL && ConstEVL->getZExtValue() == ExpectedEVL && ConstEVL->getType()->isIntegerTy(32);
> }
> ```
The first one. This isn't code golf, sir ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105283