Skip to content

[LV] Fix ScalarIVSteps vplan pattern matcher, remove m_CanonicalIV() #138298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 14, 2025
Merged

[LV] Fix ScalarIVSteps vplan pattern matcher, remove m_CanonicalIV() #138298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d527188
[LV] Fix ScalarIVSteps vplan pattern matcher, remove m_CanonicalIV()
huntergr-arm May 2, 2025
5f417a7
Add negative test
huntergr-arm May 13, 2025
75af7b4
Formatting
huntergr-arm May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 6 additions & 13 deletions llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,21 +461,14 @@ m_LogicalOr(const Op0_t &Op0, const Op1_t &Op1) {
return m_Select(Op0, m_True(), Op1);
}

using VPCanonicalIVPHI_match =
Recipe_match<std::tuple<>, 0, false, VPCanonicalIVPHIRecipe>;

inline VPCanonicalIVPHI_match m_CanonicalIV() {
return VPCanonicalIVPHI_match();
}

template <typename Op0_t, typename Op1_t>
template <typename Op0_t, typename Op1_t, typename Op2_t>
using VPScalarIVSteps_match =
Recipe_match<std::tuple<Op0_t, Op1_t>, 0, false, VPScalarIVStepsRecipe>;
TernaryRecipe_match<Op0_t, Op1_t, Op2_t, 0, false, VPScalarIVStepsRecipe>;

template <typename Op0_t, typename Op1_t>
inline VPScalarIVSteps_match<Op0_t, Op1_t> m_ScalarIVSteps(const Op0_t &Op0,
const Op1_t &Op1) {
return VPScalarIVSteps_match<Op0_t, Op1_t>(Op0, Op1);
template <typename Op0_t, typename Op1_t, typename Op2_t>
inline VPScalarIVSteps_match<Op0_t, Op1_t, Op2_t>
m_ScalarIVSteps(const Op0_t &Op0, const Op1_t &Op1, const Op2_t &Op2) {
return VPScalarIVSteps_match<Op0_t, Op1_t, Op2_t>({Op0, Op1, Op2});
}

template <typename Op0_t, typename Op1_t, typename Op2_t>
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ bool vputils::isHeaderMask(const VPValue *V, VPlan &Plan) {

if (match(V, m_ActiveLaneMask(m_VPValue(A), m_VPValue(B))))
return B == Plan.getTripCount() &&
(match(A, m_ScalarIVSteps(m_CanonicalIV(), m_SpecificInt(1))) ||
(match(A, m_ScalarIVSteps(m_Specific(Plan.getCanonicalIV()),
m_SpecificInt(1),
m_Specific(&Plan.getVF()))) ||
IsWideCanonicalIV(A));

return match(V, m_Binary<Instruction::ICmp>(m_VPValue(A), m_VPValue(B))) &&
Expand Down
1 change: 1 addition & 0 deletions llvm/unittests/Transforms/Vectorize/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ add_llvm_unittest(VectorizeTests
VPlanTest.cpp
VPDomTreeTest.cpp
VPlanHCFGTest.cpp
VPlanPatternMatchTest.cpp
VPlanSlpTest.cpp
VPlanVerifierTest.cpp
)
55 changes: 55 additions & 0 deletions llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
//===- llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp ------===//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the description, I assume it is not possible to expose this currently with an IR unit test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like vputils::isHeaderMask is only called from collectAllHeaderMasks, explicitly on users of WideCanonicalIVs. So I guess something changed at some point such that we never executed that path on recipes besides a wide canonical iv, and those matchers are now effectively unused.

collectAllHeaderMasks itself is only called from VPlanTransforms::addActiveLaneMask, which contains an assert that there is a VPWidenCanonicalIVRecipe among the users of the canonical IV.

The reason I encountered this is trying to use the matchers for my early-exit-with-store PR, and finding they didn't work.

//
//
// 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/VPlanPatternMatch.h"
#include "../lib/Transforms/Vectorize/LoopVectorizationPlanner.h"
#include "../lib/Transforms/Vectorize/VPlan.h"
#include "../lib/Transforms/Vectorize/VPlanHelpers.h"
#include "VPlanTestBase.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/Instructions.h"
#include "gtest/gtest.h"

namespace llvm {

namespace {
using VPPatternMatchTest = VPlanTestBase;

TEST_F(VPPatternMatchTest, ScalarIVSteps) {
VPlan &Plan = getPlan();
VPBasicBlock *VPBB = Plan.createVPBasicBlock("");
VPBuilder Builder(VPBB);

IntegerType *I64Ty = IntegerType::get(C, 64);
VPValue *StartV = Plan.getOrAddLiveIn(ConstantInt::get(I64Ty, 0));
auto *CanonicalIVPHI = new VPCanonicalIVPHIRecipe(StartV, DebugLoc());
Builder.insert(CanonicalIVPHI);

VPValue *Inc = Plan.getOrAddLiveIn(ConstantInt::get(I64Ty, 1));
VPValue *VF = &Plan.getVF();
VPValue *Steps = Builder.createScalarIVSteps(
Instruction::Add, nullptr, CanonicalIVPHI, Inc, VF, DebugLoc());

VPValue *Inc2 = Plan.getOrAddLiveIn(ConstantInt::get(I64Ty, 2));
VPValue *Steps2 = Builder.createScalarIVSteps(
Instruction::Add, nullptr, CanonicalIVPHI, Inc2, VF, DebugLoc());

using namespace VPlanPatternMatch;

ASSERT_TRUE(match(Steps, m_ScalarIVSteps(m_Specific(CanonicalIVPHI),
m_SpecificInt(1), m_Specific(VF))));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth having an ASSERT_FALSE for a scalar IV steps with a step != 1?

ASSERT_FALSE(
match(Steps2, m_ScalarIVSteps(m_Specific(CanonicalIVPHI),
m_SpecificInt(1), m_Specific(VF))));
ASSERT_TRUE(match(Steps2, m_ScalarIVSteps(m_Specific(CanonicalIVPHI),
m_SpecificInt(2), m_Specific(VF))));
}

} // namespace
} // namespace llvm