[VPlan] Refine return types in VPBuilder (NFC)

[artagnon]

No description provided.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/108858.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h (+14-13)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
index 2fe9af6b0d14f5..d3056b8859a9d4 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
@@ -164,33 +164,34 @@ class VPBuilder {
     return tryInsertInstruction(
         new VPInstruction(Opcode, Operands, WrapFlags, DL, Name));
   }
-  VPValue *createNot(VPValue *Operand, DebugLoc DL = {},
-                     const Twine &Name = "") {
+  VPInstruction *createNot(VPValue *Operand, DebugLoc DL = {},
+                           const Twine &Name = "") {
     return createInstruction(VPInstruction::Not, {Operand}, DL, Name);
   }
 
-  VPValue *createAnd(VPValue *LHS, VPValue *RHS, DebugLoc DL = {},
-                     const Twine &Name = "") {
+  VPInstruction *createAnd(VPValue *LHS, VPValue *RHS, DebugLoc DL = {},
+                           const Twine &Name = "") {
     return createInstruction(Instruction::BinaryOps::And, {LHS, RHS}, DL, Name);
   }
 
-  VPValue *createOr(VPValue *LHS, VPValue *RHS, DebugLoc DL = {},
-                    const Twine &Name = "") {
+  VPInstruction *createOr(VPValue *LHS, VPValue *RHS, DebugLoc DL = {},
+                          const Twine &Name = "") {
 
     return tryInsertInstruction(new VPInstruction(
         Instruction::BinaryOps::Or, {LHS, RHS},
         VPRecipeWithIRFlags::DisjointFlagsTy(false), DL, Name));
   }
 
-  VPValue *createLogicalAnd(VPValue *LHS, VPValue *RHS, DebugLoc DL = {},
-                            const Twine &Name = "") {
+  VPInstruction *createLogicalAnd(VPValue *LHS, VPValue *RHS, DebugLoc DL = {},
+                                  const Twine &Name = "") {
     return tryInsertInstruction(
         new VPInstruction(VPInstruction::LogicalAnd, {LHS, RHS}, DL, Name));
   }
 
-  VPValue *createSelect(VPValue *Cond, VPValue *TrueVal, VPValue *FalseVal,
-                        DebugLoc DL = {}, const Twine &Name = "",
-                        std::optional<FastMathFlags> FMFs = std::nullopt) {
+  VPInstruction *
+  createSelect(VPValue *Cond, VPValue *TrueVal, VPValue *FalseVal,
+               DebugLoc DL = {}, const Twine &Name = "",
+               std::optional<FastMathFlags> FMFs = std::nullopt) {
     auto *Select =
         FMFs ? new VPInstruction(Instruction::Select, {Cond, TrueVal, FalseVal},
                                  *FMFs, DL, Name)
@@ -202,8 +203,8 @@ class VPBuilder {
   /// Create a new ICmp VPInstruction with predicate \p Pred and operands \p A
   /// and \p B.
   /// TODO: add createFCmp when needed.
-  VPValue *createICmp(CmpInst::Predicate Pred, VPValue *A, VPValue *B,
-                      DebugLoc DL = {}, const Twine &Name = "") {
+  VPInstruction *createICmp(CmpInst::Predicate Pred, VPValue *A, VPValue *B,
+                            DebugLoc DL = {}, const Twine &Name = "") {
     assert(Pred >= CmpInst::FIRST_ICMP_PREDICATE &&
            Pred <= CmpInst::LAST_ICMP_PREDICATE && "invalid predicate");
     return tryInsertInstruction(

[david-arm]

One problem I see with this patch is that it forbids any kind of optimisation such as constant folding, which may want to return a result that isn't a VPInstruction.

[david-arm]

That's why IRBuilder.h defines functions like this:

Value *CreateAnd(Value *LHS, Value *RHS, const Twine &Name = "")

[artagnon]

Yes, I'm aware of this. However, I don't know if constant-folding makes sense at the VPlan-level: in practice, we use the existing constant-folder via IRBuilder while executing the plan. For the motivation for this change, see #93998.

[fhahn]

Yes, I'm aware of this. However, I don't know if constant-folding makes sense at the VPlan-level: in practice, we use the existing constant-folder via IRBuilder while executing the plan. For the motivation for this change, see #93998.

Do you know if this would help to simplify any code that uses the result of the builders (e.g. removes unnecessary cast<>)? If it helps to simplify it would be great. I don't see VPBuilder including constant-folding any time soon, but if it doesn't help to simplify existing code, it might be better to stick with the more general type for now

[artagnon]

Do you know if this would help to simplify any code that uses the result of the builders (e.g. removes unnecessary cast<>)? If it helps to simplify it would be great.

Yes, it helps strip a cast. See latest commit.

[artagnon]

@fhahn Are you happy with the simplification, or should we not pursue this change?

[artagnon]

Reviving this PR, as the constant-folder is done.

[artagnon]

Gentle ping.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[fhahn]

LGTM, with the formatting fixed. All folding is currently done as VPlan transforms which also naturally handles recipes not constructed via the builder (and opportunities exposed through other folding).

We can always adjust as needed in the future.