[LV] Fix ScalarIVSteps vplan pattern matcher, remove m_CanonicalIV() #138298
Conversation
783a846 changed VPScalarIVStepsRecipe to take 3 arguments (adding VF explicitly) instead of 2, but didn't change the corresponding pattern matcher. This matcher was only used in vputils::isHeaderMask, and no test ever reached that function with a ScalarIVSteps recipe for the value being matched -- it was always a WideCanonicalIV. So the matcher bailed out immediately before checking arguments and asserting that the number of arguments in the recipe was the same provided by the matcher. Since the constructors for ScalarIVSteps take 3 values, we should be safe to update the matcher and guard it with a dedicated gtest. m_CanonicalIV() on the other hand is removed; as a phi recipe it may not have a consistent number of arguments to match, only requiring one (the start value) when being constructed with the assumption that a second incoming value is added for the backedge later. In order to keep the matcher we would need to add multiple matchers with different numbers of arguments for it depending on what phase of vplan construction we were in, and ensure that we never reorder matcher usage vs. vplan transformation. Since the main IR PatternMatch.h doesn't contain any matchers for PHI nodes, I think we can just remove it and match via m_Specific() using the VPValue we get from Plan.getCanonicalIV().
|
@llvm/pr-subscribers-vectorizers Author: Graham Hunter (huntergr-arm) Changes783a846 changed VPScalarIVStepsRecipe to take 3 arguments (adding This matcher was only used in vputils::isHeaderMask, and no test Since the constructors for ScalarIVSteps take 3 values, we should m_CanonicalIV() on the other hand is removed; as a phi recipe it Full diff: https://github.com/llvm/llvm-project/pull/138298.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
index 58865c296ed8a..f2a7f16e19a79 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
@@ -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>
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
index 2db4957409c8d..82b2ed242b0cb 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
@@ -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))) &&
diff --git a/llvm/unittests/Transforms/Vectorize/CMakeLists.txt b/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
index 0df39c41a9041..53eeff28c185f 100644
--- a/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
+++ b/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
@@ -12,6 +12,7 @@ add_llvm_unittest(VectorizeTests
VPlanTest.cpp
VPDomTreeTest.cpp
VPlanHCFGTest.cpp
+ VPlanPatternMatchTest.cpp
VPlanSlpTest.cpp
VPlanVerifierTest.cpp
)
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp b/llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp
new file mode 100644
index 0000000000000..e974af769ad27
--- /dev/null
+++ b/llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp
@@ -0,0 +1,46 @@
+//===- llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.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/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());
+
+ using namespace VPlanPatternMatch;
+
+ ASSERT_TRUE(match(Steps, m_ScalarIVSteps(m_Specific(CanonicalIVPHI),
+ m_SpecificInt(1), m_Specific(VF))));
+}
+
+} // namespace
+} // namespace llvm
|
|
@llvm/pr-subscribers-llvm-transforms Author: Graham Hunter (huntergr-arm) Changes783a846 changed VPScalarIVStepsRecipe to take 3 arguments (adding This matcher was only used in vputils::isHeaderMask, and no test Since the constructors for ScalarIVSteps take 3 values, we should m_CanonicalIV() on the other hand is removed; as a phi recipe it Full diff: https://github.com/llvm/llvm-project/pull/138298.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
index 58865c296ed8a..f2a7f16e19a79 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
@@ -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>
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
index 2db4957409c8d..82b2ed242b0cb 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
@@ -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))) &&
diff --git a/llvm/unittests/Transforms/Vectorize/CMakeLists.txt b/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
index 0df39c41a9041..53eeff28c185f 100644
--- a/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
+++ b/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
@@ -12,6 +12,7 @@ add_llvm_unittest(VectorizeTests
VPlanTest.cpp
VPDomTreeTest.cpp
VPlanHCFGTest.cpp
+ VPlanPatternMatchTest.cpp
VPlanSlpTest.cpp
VPlanVerifierTest.cpp
)
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp b/llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp
new file mode 100644
index 0000000000000..e974af769ad27
--- /dev/null
+++ b/llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp
@@ -0,0 +1,46 @@
+//===- llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.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/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());
+
+ using namespace VPlanPatternMatch;
+
+ ASSERT_TRUE(match(Steps, m_ScalarIVSteps(m_Specific(CanonicalIVPHI),
+ m_SpecificInt(1), m_Specific(VF))));
+}
+
+} // namespace
+} // namespace llvm
|
| @@ -0,0 +1,46 @@ | |||
| //===- llvm/unittests/Transforms/Vectorize/VPlanPatternMatchTest.cpp ------===// | |||
There was a problem hiding this comment.
From the description, I assume it is not possible to expose this currently with an IR unit test?
There was a problem hiding this comment.
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.
| using namespace VPlanPatternMatch; | ||
|
|
||
| ASSERT_TRUE(match(Steps, m_ScalarIVSteps(m_Specific(CanonicalIVPHI), | ||
| m_SpecificInt(1), m_Specific(VF)))); |
There was a problem hiding this comment.
Is it worth having an ASSERT_FALSE for a scalar IV steps with a step != 1?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/202/builds/1236 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/146/builds/2921 Here is the relevant piece of the build log for the reference |
783a846 changed VPScalarIVStepsRecipe to take 3 arguments (adding
VF explicitly) instead of 2, but didn't change the corresponding
pattern matcher.
This matcher was only used in vputils::isHeaderMask, and no test
ever reached that function with a ScalarIVSteps recipe for the
value being matched -- it was always a WideCanonicalIV. So the
matcher bailed out immediately before checking arguments and
asserting that the number of arguments in the recipe was the
same provided by the matcher.
Since the constructors for ScalarIVSteps take 3 values, we should
be safe to update the matcher and guard it with a dedicated gtest.
m_CanonicalIV() on the other hand is removed; as a phi recipe it
may not have a consistent number of arguments to match, only
requiring one (the start value) when being constructed with the
assumption that a second incoming value is added for the backedge
later. In order to keep the matcher we would need to add multiple
matchers with different numbers of arguments for it depending on
what phase of vplan construction we were in, and ensure that we
never reorder matcher usage vs. vplan transformation. Since the
main IR PatternMatch.h doesn't contain any matchers for PHI nodes,
I think we can just remove it and match via m_Specific() using the
VPValue we get from Plan.getCanonicalIV().