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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/VectorBuilder.h:26
+
+using ValArray = ArrayRef<Value *>;
+
----------------
Do we really need this typedef? Mainly asking because it went into the llvm namespace.


================
Comment at: llvm/include/llvm/IR/VectorBuilder.h:28
+
+enum VectorBuilderBehavior {
+  // Abort if the requested VP intrinsic could not be created.
----------------
Can we move this into the class so it becomes VectorBuilder::Behavior?


================
Comment at: llvm/include/llvm/IR/VectorBuilder.h:83
+  VectorBuilder &setStaticVL(unsigned NewFixedVL) {
+    StaticVectorLength = ElementCount::getFixed(NewFixedVL);
+    return *this;
----------------
How do you set StaticVectorLength to a scalable value?


================
Comment at: llvm/lib/IR/VectorBuilder.cpp:75
+    if (ParamIdx < InstOpArray.size())
+      IntrinParams.push_back(InstOpArray[ParamIdx]);
+  }
----------------
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.


================
Comment at: llvm/unittests/IR/VectorBuilderTest.cpp:21
+
+static int VectorNumElements = 8;
+
----------------
nit: int->unsigned. That's at least what most of the interfaces this gets passed to use.


================
Comment at: llvm/unittests/IR/VectorBuilderTest.cpp:31
+                                              Value *&Mask, Value *&EVL) {
+    auto *Mod = new Module("TestModule", Context);
+    auto *Int32Ty = Type::getInt32Ty(Context);
----------------
Use std::make_unique instead of creating a unique_ptr for the return.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105283