[LV] Provide utility routine to find uncounted exit recipes #152530
Conversation
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Graham Hunter (huntergr-arm) ChangesSplitting out just the recipe finding code from #148626 into a utility function (along with the extra pattern matchers). Hopefully this makes reviewing a bit easier. Added a gtest, since this isn't actually used anywhere yet. Full diff: https://github.com/llvm/llvm-project/pull/152530.diff 6 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
index 8818843a30625..d7f9763c4d0c8 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
@@ -692,6 +692,37 @@ m_Intrinsic(const T0 &Op0, const T1 &Op1, const T2 &Op2, const T3 &Op3) {
return m_CombineAnd(m_Intrinsic<IntrID>(Op0, Op1, Op2), m_Argument<3>(Op3));
}
+struct loop_invariant_vpvalue {
+ template <typename ITy> bool match(ITy *V) const {
+ VPValue *Val = dyn_cast<VPValue>(V);
+ return Val && Val->isDefinedOutsideLoopRegions();
+ }
+};
+
+inline loop_invariant_vpvalue m_LoopInvVPValue() {
+ return loop_invariant_vpvalue();
+}
+
+template <typename Op0_t>
+inline UnaryVPInstruction_match<Op0_t, VPInstruction::AnyOf>
+m_AnyOf(const Op0_t &Op0) {
+ return m_VPInstruction<VPInstruction::AnyOf>(Op0);
+}
+
+template <typename SubPattern_t> struct OneUse_match {
+ SubPattern_t SubPattern;
+
+ OneUse_match(const SubPattern_t &SP) : SubPattern(SP) {}
+
+ template <typename OpTy> bool match(OpTy *V) {
+ return V->hasOneUse() && SubPattern.match(V);
+ }
+};
+
+template <typename T> inline OneUse_match<T> m_OneUse(const T &SubPattern) {
+ return SubPattern;
+}
+
} // namespace VPlanPatternMatch
} // namespace llvm
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
index 14f20c65a7034..358c38f49405c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
@@ -138,3 +138,103 @@ VPBasicBlock *vputils::getFirstLoopHeader(VPlan &Plan, VPDominatorTree &VPDT) {
});
return I == DepthFirst.end() ? nullptr : cast<VPBasicBlock>(*I);
}
+
+std::optional<VPValue *> vputils::getRecipesForUncountedExit(
+ VPlan &Plan, SmallVectorImpl<VPRecipeBase *> &Recipes,
+ SmallVectorImpl<VPReplicateRecipe *> &GEPs) {
+ using namespace llvm::VPlanPatternMatch;
+ // Given a vplan like the following (just including the recipes contributing
+ // to loop control exiting here, not the actual work), we're looking to match
+ // the recipes contributing to the uncounted exit condition comparison
+ // (here, vp<%4>) back to the canonical induction for the vector body so that
+ // we can copy them to a preheader and rotate the address in the loop to the
+ // next vector iteration.
+ //
+ // VPlan ' for UF>=1' {
+ // Live-in vp<%0> = VF
+ // Live-in ir<64> = original trip-count
+ //
+ // entry:
+ // Successor(s): preheader, vector.ph
+ //
+ // vector.ph:
+ // Successor(s): vector loop
+ //
+ // <x1> vector loop: {
+ // vector.body:
+ // EMIT vp<%2> = CANONICAL-INDUCTION ir<0>
+ // vp<%3> = SCALAR-STEPS vp<%2>, ir<1>, vp<%0>
+ // CLONE ir<%ee.addr> = getelementptr ir<0>, vp<%3>
+ // WIDEN ir<%ee.load> = load ir<%ee.addr>
+ // WIDEN vp<%4> = icmp eq ir<%ee.load>, ir<0>
+ // EMIT vp<%5> = any-of vp<%4>
+ // EMIT vp<%6> = add vp<%2>, vp<%0>
+ // EMIT vp<%7> = icmp eq vp<%6>, ir<64>
+ // EMIT vp<%8> = or vp<%5>, vp<%7>
+ // EMIT branch-on-cond vp<%8>
+ // No successors
+ // }
+ // Successor(s): middle.block
+ //
+ // middle.block:
+ // Successor(s): preheader
+ //
+ // preheader:
+ // No successors
+ // }
+
+ // Find the uncounted loop exit condition.
+ auto *Region = Plan.getVectorLoopRegion();
+ VPValue *UncountedCondition = nullptr;
+ if (!match(
+ Region->getExitingBasicBlock()->getTerminator(),
+ m_BranchOnCond(m_OneUse(m_c_BinaryOr(
+ m_OneUse(m_AnyOf(m_VPValue(UncountedCondition))), m_VPValue())))))
+ return std::nullopt;
+
+ SmallVector<VPValue *, 4> Worklist;
+ bool LoadFound = false;
+ Worklist.push_back(UncountedCondition);
+ while (!Worklist.empty()) {
+ VPValue *V = Worklist.pop_back_val();
+
+ if (V->isDefinedOutsideLoopRegions())
+ continue;
+ if (V->getNumUsers() > 1)
+ return std::nullopt;
+
+ if (auto *Cmp = dyn_cast<VPWidenRecipe>(V)) {
+ if (Cmp->getOpcode() != Instruction::ICmp)
+ return std::nullopt;
+ Worklist.push_back(Cmp->getOperand(0));
+ Worklist.push_back(Cmp->getOperand(1));
+ Recipes.push_back(Cmp);
+ } else if (auto *Load = dyn_cast<VPWidenLoadRecipe>(V)) {
+ if (!Load->isConsecutive() || Load->isMasked())
+ return std::nullopt;
+ Worklist.push_back(Load->getAddr());
+ Recipes.push_back(Load);
+ LoadFound = true;
+ } else if (auto *VecPtr = dyn_cast<VPVectorPointerRecipe>(V)) {
+ Worklist.push_back(VecPtr->getOperand(0));
+ Recipes.push_back(VecPtr);
+ } else if (auto *GEP = dyn_cast<VPReplicateRecipe>(V)) {
+ if (GEP->getNumOperands() != 2)
+ return std::nullopt;
+ if (!match(GEP, m_GetElementPtr(
+ m_LoopInvVPValue(),
+ m_ScalarIVSteps(m_Specific(Plan.getCanonicalIV()),
+ m_SpecificInt(1),
+ m_Specific(&Plan.getVF())))))
+ return std::nullopt;
+ GEPs.push_back(GEP);
+ Recipes.push_back(GEP);
+ } else
+ return std::nullopt;
+ }
+
+ if (GEPs.empty() || !LoadFound)
+ return std::nullopt;
+
+ return UncountedCondition;
+}
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUtils.h b/llvm/lib/Transforms/Vectorize/VPlanUtils.h
index 8dcd57f1b3598..631d7aa8da9ee 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUtils.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanUtils.h
@@ -97,6 +97,14 @@ bool isUniformAcrossVFsAndUFs(VPValue *V);
/// Returns the header block of the first, top-level loop, or null if none
/// exist.
VPBasicBlock *getFirstLoopHeader(VPlan &Plan, VPDominatorTree &VPDT);
+
+/// Returns the VPValue representing the uncounted exit comparison if all the
+/// recipes needed to form the condition within the vector loop body were
+/// matched.
+std::optional<VPValue *>
+getRecipesForUncountedExit(VPlan &Plan,
+ SmallVectorImpl<VPRecipeBase *> &Recipes,
+ SmallVectorImpl<VPReplicateRecipe *> &GEPs);
} // namespace vputils
//===----------------------------------------------------------------------===//
diff --git a/llvm/lib/Transforms/Vectorize/VPlanValue.h b/llvm/lib/Transforms/Vectorize/VPlanValue.h
index 24f6d61512ef6..5fecbbdef4b5b 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanValue.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanValue.h
@@ -148,6 +148,8 @@ class LLVM_ABI_FOR_TEST VPValue {
return Current != user_end();
}
+ bool hasOneUse() const { return getNumUsers() == 1; }
+
void replaceAllUsesWith(VPValue *New);
/// Go through the uses list for this VPValue and make each use point to \p
diff --git a/llvm/unittests/Transforms/Vectorize/CMakeLists.txt b/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
index 53eeff28c185f..a7254922af007 100644
--- a/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
+++ b/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
@@ -14,5 +14,6 @@ add_llvm_unittest(VectorizeTests
VPlanHCFGTest.cpp
VPlanPatternMatchTest.cpp
VPlanSlpTest.cpp
+ VPlanUncountedExitTest.cpp
VPlanVerifierTest.cpp
)
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanUncountedExitTest.cpp b/llvm/unittests/Transforms/Vectorize/VPlanUncountedExitTest.cpp
new file mode 100644
index 0000000000000..81ef67a0fb923
--- /dev/null
+++ b/llvm/unittests/Transforms/Vectorize/VPlanUncountedExitTest.cpp
@@ -0,0 +1,99 @@
+//===- llvm/unittests/Transforms/Vectorize/VPlanUncountedExitTest.cpp -----===//
+//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "../lib/Transforms/Vectorize/LoopVectorizationPlanner.h"
+#include "../lib/Transforms/Vectorize/VPlan.h"
+#include "../lib/Transforms/Vectorize/VPlanPatternMatch.h"
+#include "../lib/Transforms/Vectorize/VPlanUtils.h"
+#include "VPlanTestBase.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/IR/Instruction.h"
+#include "llvm/IR/Instructions.h"
+#include "gtest/gtest.h"
+
+namespace llvm {
+
+namespace {
+class VPUncountedExitTest : public VPlanTestBase {};
+
+TEST_F(VPUncountedExitTest, FindUncountedExitRecipes) {
+ // Create CFG skeleton.
+ VPlan &Plan = getPlan();
+ VPBasicBlock *ScalarPH = Plan.getEntry();
+ VPBasicBlock *Entry = Plan.createVPBasicBlock("entry");
+ Plan.setEntry(Entry);
+ VPBasicBlock *VectorPH = Plan.createVPBasicBlock("vector.ph");
+ VPBasicBlock *VecBody = Plan.createVPBasicBlock("vector.body");
+ VPRegionBlock *Region =
+ Plan.createVPRegionBlock(VecBody, VecBody, "vector loop");
+ VPBasicBlock *MiddleBlock = Plan.createVPBasicBlock("middle.block");
+ VPBlockUtils::connectBlocks(Entry, ScalarPH);
+ VPBlockUtils::connectBlocks(Entry, VectorPH);
+ VPBlockUtils::connectBlocks(VectorPH, Region);
+ VPBlockUtils::connectBlocks(Region, MiddleBlock);
+ VPBlockUtils::connectBlocks(MiddleBlock, ScalarPH);
+
+ // Live-Ins
+ IntegerType *I64Ty = IntegerType::get(C, 64);
+ IntegerType *I32Ty = IntegerType::get(C, 32);
+ PointerType *PTy = PointerType::get(C, 0);
+ VPValue *Zero = Plan.getOrAddLiveIn(ConstantInt::get(I64Ty, 0));
+ VPValue *Inc = Plan.getOrAddLiveIn(ConstantInt::get(I64Ty, 1));
+ VPValue *VF = &Plan.getVF();
+ Plan.setTripCount(Plan.getOrAddLiveIn(ConstantInt::get(I64Ty, 64)));
+
+ // Populate vector.body with the recipes for exiting.
+ auto *IV = new VPCanonicalIVPHIRecipe(Zero, {});
+ VecBody->appendRecipe(IV);
+ VPBuilder Builder(VecBody, VecBody->getFirstNonPhi());
+ auto *Steps = Builder.createScalarIVSteps(Instruction::Add, nullptr, IV, Inc,
+ VF, DebugLoc());
+
+ // Uncounted Exit; GEP -> Load -> Cmp
+ auto *DummyGEP = GetElementPtrInst::Create(I32Ty, Zero->getUnderlyingValue(),
+ {}, Twine("ee.addr"));
+ auto *GEP = new VPReplicateRecipe(DummyGEP, {Zero, Steps}, true, nullptr);
+ Builder.insert(GEP);
+ auto *DummyLoad =
+ new LoadInst(I32Ty, PoisonValue::get(PTy), "ee.load", false, Align(1));
+ VPValue *Load =
+ new VPWidenLoadRecipe(*DummyLoad, GEP, nullptr, true, false, {}, {});
+ Builder.insert(Load->getDefiningRecipe());
+ // Should really splat the zero, but we're not checking types here.
+ VPValue *Cmp = new VPWidenRecipe(Instruction::ICmp, {Load, Zero},
+ VPIRFlags(CmpInst::ICMP_EQ), {}, {});
+ Builder.insert(Cmp->getDefiningRecipe());
+ VPValue *AnyOf = Builder.createNaryOp(VPInstruction::AnyOf, Cmp);
+
+ // Counted Exit; Inc IV -> Cmp
+ VPValue *NextIV = Builder.createNaryOp(Instruction::Add, {IV, VF});
+ VPValue *Counted =
+ Builder.createICmp(CmpInst::ICMP_EQ, NextIV, Plan.getTripCount());
+
+ // Combine, and branch.
+ VPValue *Combined = Builder.createNaryOp(Instruction::Or, {AnyOf, Counted});
+ Builder.createNaryOp(VPInstruction::BranchOnCond, {Combined});
+
+ SmallVector<VPRecipeBase *, 8> Recipes;
+ SmallVector<VPReplicateRecipe *, 2> GEPs;
+
+ std::optional<VPValue *> UncountedCondition =
+ vputils::getRecipesForUncountedExit(Plan, Recipes, GEPs);
+ ASSERT_TRUE(UncountedCondition.has_value());
+ ASSERT_EQ(*UncountedCondition, Cmp);
+ ASSERT_EQ(GEPs.size(), 1ull);
+ ASSERT_EQ(GEPs[0], GEP);
+ ASSERT_EQ(Recipes.size(), 3ull);
+
+ delete DummyLoad;
+ delete DummyGEP;
+}
+
+} // namespace
+} // namespace llvm
|
There was a problem hiding this comment.
The name of the function doesn't match the code, i.e. one use != one user. I think it should either be:
bool hasOneUser() const { return getNumUsers() == 1; }
or
bool hasOneUse() const { return getNumUses() == 1; }
There was a problem hiding this comment.
Uses are not directly modeled in VPlan, afaict.
Instead, a User may be recorded multiple times for a given VPValue. See the function directly above this, hasMoreThanOneUniqueUser, which looks through the list of Users seeing if there's one that's different from the first.
There was a problem hiding this comment.
Hi @fhahn, I think this is pretty confusing. Can we rename the existing getNumUsers to getNumUses to more closely match the naming conventions used for IR values? If a recipe has duplicate input operands then the number of uses != 1, but the number of users may still be 1. If so, I'd like to put up a patch to tidy this up a little.
There was a problem hiding this comment.
Is this function only ever intended to be called before we handle the uncountable exit, i.e. before we've introduced the middle split block? If it's after then the vplan in the comments below probably needs updating to reference the middle.split block.
It might be good to clarify in the comments where you expect this function to be called, as it seems to require the vplan CFG to be in a particular format.
There was a problem hiding this comment.
It's intended to be called after the vplan has been created and the CFG simplified so there's only one exit. This is currently after the existing uncounted early exit transformation, but that is changed in the transform PR to not create the split.
We probably want to formalize the strategy as state in the VPlan so we know which parts of the transformations to apply.
There was a problem hiding this comment.
Does it matter if this is a loop with an uncountable early exit that doesn't match this pattern? If it's being called as part of VPlanTransforms::handleEarlyExits then it should be fine, but later on I can imagine a VPlanTransform may optimise some of this code. I assume that returning std::nullopt would mean it's game over for vectorisation of a loop with an uncountable early exit and a store?
There was a problem hiding this comment.
If we can't match the expected recipes here, then the transform (from #148626) will abandon the vplan and vectorization will not proceed.
It's the reason I can do this as a vplan transform instead of manually planting recipes as part of the initial vplan creation based on what LoopVectorizationLegality finds -- I did it the latter way in 2015 when I originally prototyped early exit autovec.
There was a problem hiding this comment.
The IR above suggests we've already called handleUncountableEarlyExit and so this probably should be // Successor(s): middle.split
There was a problem hiding this comment.
The transform PR explicitly avoids creating the split, since we won't have finished all iterations before leaving the vector body.
There was a problem hiding this comment.
In the general case, LoadFound could be set to true for a vplan where neither icmp input is a load, but you just happen to bump into a load in the loop before the early exit. Are you specifically interested in finding any load in the loop, or finding a specific load, i.e. the load that forms the input to the icmp? If it's the latter I think you need to check that the load forms the input to the icmp before setting the boolean to true.
I realise that some of the legality work you're doing requires something of the form:
%load = load i32, ptr %gep
%icmp = icmp i32 %load, 3
but I don't think you should assume that here.
There was a problem hiding this comment.
I only consider operands from each recipe; that's what the worklist is for. So I start with the input to AnyOf above, which happens to be an icmp in the loop I'm using for the prototype, confirm that it's valid then add the two operands to the worklist.
In the case of the test, one of those operands is a live-in constant, so is accepted immediately. The other is the load, which is checked and the address recipe is added to the worklist.
I am moving the load checking to be more strict for now, as discussed with Sander above.
There was a problem hiding this comment.
Well, I was worried about perfectly valid recipes like this that you might encounter:
%icmp1 = icmp %load1, %load2
%icmp2 = icmp %load3, %load4
%icmp3 = icmp %icmp1, %icmp2
%any_of = any_of %icmp3
There was a problem hiding this comment.
Yeah, that should be fine. As long as all the recipes contributing to the exit condition can be found and tracked back to the canonical IV or a live-in I don't see a problem with us matching more recipes.
If you want me to restrict it to a single comparison right now I can do so, but I wanted to show the basis of the transform approach -- using a worklist to handle vplan recipes instead of using IR directly during initial plan construction.
There was a problem hiding this comment.
nit: Is there a reason this check is done after the above loop using a Loads array, rather than checking it right where it encounters the VPWidenLoadRecipe above?
There was a problem hiding this comment.
I think you can use the new m_ICmp matchers here
There was a problem hiding this comment.
| // Given a vplan like the following (just including the recipes contributing | |
| // Given a VPlan like the following (just including the recipes contributing |
There was a problem hiding this comment.
Do we need to check for the restriction here? Would it be better to check them in the caller? If not, it would be good to document the restrictions in the doc-comment
There was a problem hiding this comment.
| vputils::getRecipesForUncountedExit(VPlan &Plan, | |
| vputils::getRecipesForUncountableExit(VPlan &Plan, |
Uncounted -> Uncountable throughout to keep consistent with the existing terminology
There was a problem hiding this comment.
Can we use VPTestBase::buildVPlan to construct the initial VPlan from a simple CFG loop and then just create convert the Load VPInstructions to VPWidenLoadRecipes? That would ensure we keep things in-sync if the constructed VPlans change.
huntergr-arm
force-pushed
the
identify-uncounted-exit-condition-recipes
branch
from
2debe28 to
ebdb2b6
Compare
huntergr-arm
force-pushed
the
identify-uncounted-exit-condition-recipes
branch
from
ebdb2b6 to
6f7ca85
Compare
|
Rebased, will commit next week if there's no further comments. |
There was a problem hiding this comment.
| /// the canonical IV and it is deemed safe to copy those recipes into the | |
| /// vector preheader. The recipes are stored in \p Recipes, and recipes | |
| /// the canonical IV. The recipes are stored in \p Recipes, and recipes |
I think the latest version doesn't check this?
There was a problem hiding this comment.
Should this mention loads as well? Looks like we stop also stop at them
There was a problem hiding this comment.
Should probably also mention the special treatment of masked loads (or remove it)
There was a problem hiding this comment.
I've changed it to note that it stops at the addresses of non-masked loads.
There was a problem hiding this comment.
Do we hit pointer bases with recipes that are moved out of the vector loop in practice or would m_LiveIn suffice?
There was a problem hiding this comment.
Changed to m_LiveIn, seems to work fine.
There was a problem hiding this comment.
can you move this to the other VPInstruction matchers for consitency?
huntergr-arm
force-pushed
the
identify-uncounted-exit-condition-recipes
branch
from
6f7ca85 to
2f81520
Compare
| // we can copy them to a preheader and rotate the address in the loop to the | ||
| // next vector iteration. | ||
| // | ||
| // Currently, the address of the load is restricted to a GEP with 2 terms and |
There was a problem hiding this comment.
| // Currently, the address of the load is restricted to a GEP with 2 terms and | |
| // Currently, the address of the load is restricted to a GEP with 2 operands and |
| // Given a VPlan like the following (just including the recipes contributing | ||
| // to loop control exiting here, not the actual work), we're looking to match | ||
| // the recipes contributing to the uncountable exit condition comparison | ||
| // (here, vp<%4>) back to the canonical induction for the vector body so that |
There was a problem hiding this comment.
It looks this may be out-of-sync with the implementation, it looks like we don't check for a canonical induction, just for loads & live-ins as in the header comment?
| SmallVector<VPRecipeBase *, 8> Recipes; | ||
| SmallVector<VPRecipeBase *, 2> GEPs; | ||
|
|
||
| std::optional<VPValue *> UncountableCondition = |
There was a problem hiding this comment.
it would also be good to add a test without an early exit, to make sure that code path is also covered by the test
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
LGTM, thanks! A few small suggestions left inline
llvm/unittests/Transforms/Vectorize/VPlanUncountableExitTest.cpp
Outdated
Show resolved
Hide resolved
llvm/unittests/Transforms/Vectorize/VPlanUncountableExitTest.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Florian Hahn <[email protected]>
Co-authored-by: Florian Hahn <[email protected]>
Co-authored-by: Florian Hahn <[email protected]>
|
Thanks for the review! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/16223 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/22816 Here is the relevant piece of the build log for the reference |
6 participants
Splitting out just the recipe finding code from #148626 into a utility function (along with the extra pattern matchers). Hopefully this makes reviewing a bit easier.
Added a gtest, since this isn't actually used anywhere yet.