Reland [EquivClasses] Introduce members iterator-helper #130319
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Changes: Fix the expectations in EquivalenceClassesTest.MemberIterator, also fixing a build failure.
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Ramkumar Ramachandra (artagnon) ChangesChanges: Fix the expectations in EquivalenceClassesTest.MemberIterator, also fixing a build failure. -- 8< -- Full diff: https://github.com/llvm/llvm-project/pull/130319.diff 4 Files Affected:
diff --git a/llvm/include/llvm/ADT/EquivalenceClasses.h b/llvm/include/llvm/ADT/EquivalenceClasses.h
index 4f98b84cf97d2..345107c7777b0 100644
--- a/llvm/include/llvm/ADT/EquivalenceClasses.h
+++ b/llvm/include/llvm/ADT/EquivalenceClasses.h
@@ -15,6 +15,7 @@
#ifndef LLVM_ADT_EQUIVALENCECLASSES_H
#define LLVM_ADT_EQUIVALENCECLASSES_H
+#include "llvm/ADT/iterator_range.h"
#include <cassert>
#include <cstddef>
#include <cstdint>
@@ -178,6 +179,9 @@ class EquivalenceClasses {
member_iterator member_end() const {
return member_iterator(nullptr);
}
+ iterator_range<member_iterator> members(iterator I) const {
+ return make_range(member_begin(I), member_end());
+ }
/// findValue - Return an iterator to the specified value. If it does not
/// exist, end() is returned.
diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index 38ee82b77a946..207f5417934e5 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -527,9 +527,8 @@ void RuntimePointerChecking::groupChecks(
// iteration order within an equivalence class member is only dependent on
// the order in which unions and insertions are performed on the
// equivalence class, the iteration order is deterministic.
- for (auto MI = DepCands.member_begin(LeaderI), ME = DepCands.member_end();
- MI != ME; ++MI) {
- auto PointerI = PositionMap.find(MI->getPointer());
+ for (const auto &MI : DepCands.members(LeaderI)) {
+ auto PointerI = PositionMap.find(MI.getPointer());
assert(PointerI != PositionMap.end() &&
"pointer in equivalence class not found in PositionMap");
for (unsigned Pointer : PointerI->second) {
diff --git a/llvm/lib/Analysis/VectorUtils.cpp b/llvm/lib/Analysis/VectorUtils.cpp
index 91ba68fe03324..c0bc451973c6e 100644
--- a/llvm/lib/Analysis/VectorUtils.cpp
+++ b/llvm/lib/Analysis/VectorUtils.cpp
@@ -845,7 +845,7 @@ llvm::computeMinimumValueSizes(ArrayRef<BasicBlock *> Blocks, DemandedBits &DB,
for (auto I = ECs.begin(), E = ECs.end(); I != E; ++I) {
uint64_t LeaderDemandedBits = 0;
- for (Value *M : llvm::make_range(ECs.member_begin(I), ECs.member_end()))
+ for (Value *M : ECs.members(I))
LeaderDemandedBits |= DBits[M];
uint64_t MinBW = llvm::bit_width(LeaderDemandedBits);
@@ -857,7 +857,7 @@ llvm::computeMinimumValueSizes(ArrayRef<BasicBlock *> Blocks, DemandedBits &DB,
// indvars.
// If we are required to shrink a PHI, abandon this entire equivalence class.
bool Abort = false;
- for (Value *M : llvm::make_range(ECs.member_begin(I), ECs.member_end()))
+ for (Value *M : ECs.members(I))
if (isa<PHINode>(M) && MinBW < M->getType()->getScalarSizeInBits()) {
Abort = true;
break;
@@ -865,7 +865,7 @@ llvm::computeMinimumValueSizes(ArrayRef<BasicBlock *> Blocks, DemandedBits &DB,
if (Abort)
continue;
- for (Value *M : llvm::make_range(ECs.member_begin(I), ECs.member_end())) {
+ for (Value *M : ECs.members(I)) {
auto *MI = dyn_cast<Instruction>(M);
if (!MI)
continue;
diff --git a/llvm/unittests/ADT/EquivalenceClassesTest.cpp b/llvm/unittests/ADT/EquivalenceClassesTest.cpp
index 70e161a03d988..c24c09d8a2815 100644
--- a/llvm/unittests/ADT/EquivalenceClassesTest.cpp
+++ b/llvm/unittests/ADT/EquivalenceClassesTest.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/ADT/EquivalenceClasses.h"
+#include "gmock/gmock.h"
#include "gtest/gtest.h"
using namespace llvm;
@@ -66,6 +67,19 @@ TEST(EquivalenceClassesTest, TwoSets) {
EXPECT_FALSE(EqClasses.isEquivalent(i, j));
}
+TEST(EquivalenceClassesTest, MembersIterator) {
+ EquivalenceClasses<int> EC;
+ EC.unionSets(1, 2);
+ EC.insert(4);
+ EC.insert(5);
+ EC.unionSets(5, 1);
+ EXPECT_EQ(EC.getNumClasses(), 2u);
+
+ EquivalenceClasses<int>::iterator I = EC.findValue(EC.getLeaderValue(1));
+ EXPECT_THAT(EC.members(I), testing::ElementsAre(5, 1, 2));
+ EXPECT_EQ(EC.members(EC.end()).begin(), EC.member_end());
+}
+
// Type-parameterized tests: Run the same test cases with different element
// types.
template <typename T> class ParameterizedTest : public testing::Test {};
|
|
@llvm/pr-subscribers-llvm-adt Author: Ramkumar Ramachandra (artagnon) ChangesChanges: Fix the expectations in EquivalenceClassesTest.MemberIterator, also fixing a build failure. -- 8< -- Full diff: https://github.com/llvm/llvm-project/pull/130319.diff 4 Files Affected:
diff --git a/llvm/include/llvm/ADT/EquivalenceClasses.h b/llvm/include/llvm/ADT/EquivalenceClasses.h
index 4f98b84cf97d2..345107c7777b0 100644
--- a/llvm/include/llvm/ADT/EquivalenceClasses.h
+++ b/llvm/include/llvm/ADT/EquivalenceClasses.h
@@ -15,6 +15,7 @@
#ifndef LLVM_ADT_EQUIVALENCECLASSES_H
#define LLVM_ADT_EQUIVALENCECLASSES_H
+#include "llvm/ADT/iterator_range.h"
#include <cassert>
#include <cstddef>
#include <cstdint>
@@ -178,6 +179,9 @@ class EquivalenceClasses {
member_iterator member_end() const {
return member_iterator(nullptr);
}
+ iterator_range<member_iterator> members(iterator I) const {
+ return make_range(member_begin(I), member_end());
+ }
/// findValue - Return an iterator to the specified value. If it does not
/// exist, end() is returned.
diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index 38ee82b77a946..207f5417934e5 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -527,9 +527,8 @@ void RuntimePointerChecking::groupChecks(
// iteration order within an equivalence class member is only dependent on
// the order in which unions and insertions are performed on the
// equivalence class, the iteration order is deterministic.
- for (auto MI = DepCands.member_begin(LeaderI), ME = DepCands.member_end();
- MI != ME; ++MI) {
- auto PointerI = PositionMap.find(MI->getPointer());
+ for (const auto &MI : DepCands.members(LeaderI)) {
+ auto PointerI = PositionMap.find(MI.getPointer());
assert(PointerI != PositionMap.end() &&
"pointer in equivalence class not found in PositionMap");
for (unsigned Pointer : PointerI->second) {
diff --git a/llvm/lib/Analysis/VectorUtils.cpp b/llvm/lib/Analysis/VectorUtils.cpp
index 91ba68fe03324..c0bc451973c6e 100644
--- a/llvm/lib/Analysis/VectorUtils.cpp
+++ b/llvm/lib/Analysis/VectorUtils.cpp
@@ -845,7 +845,7 @@ llvm::computeMinimumValueSizes(ArrayRef<BasicBlock *> Blocks, DemandedBits &DB,
for (auto I = ECs.begin(), E = ECs.end(); I != E; ++I) {
uint64_t LeaderDemandedBits = 0;
- for (Value *M : llvm::make_range(ECs.member_begin(I), ECs.member_end()))
+ for (Value *M : ECs.members(I))
LeaderDemandedBits |= DBits[M];
uint64_t MinBW = llvm::bit_width(LeaderDemandedBits);
@@ -857,7 +857,7 @@ llvm::computeMinimumValueSizes(ArrayRef<BasicBlock *> Blocks, DemandedBits &DB,
// indvars.
// If we are required to shrink a PHI, abandon this entire equivalence class.
bool Abort = false;
- for (Value *M : llvm::make_range(ECs.member_begin(I), ECs.member_end()))
+ for (Value *M : ECs.members(I))
if (isa<PHINode>(M) && MinBW < M->getType()->getScalarSizeInBits()) {
Abort = true;
break;
@@ -865,7 +865,7 @@ llvm::computeMinimumValueSizes(ArrayRef<BasicBlock *> Blocks, DemandedBits &DB,
if (Abort)
continue;
- for (Value *M : llvm::make_range(ECs.member_begin(I), ECs.member_end())) {
+ for (Value *M : ECs.members(I)) {
auto *MI = dyn_cast<Instruction>(M);
if (!MI)
continue;
diff --git a/llvm/unittests/ADT/EquivalenceClassesTest.cpp b/llvm/unittests/ADT/EquivalenceClassesTest.cpp
index 70e161a03d988..c24c09d8a2815 100644
--- a/llvm/unittests/ADT/EquivalenceClassesTest.cpp
+++ b/llvm/unittests/ADT/EquivalenceClassesTest.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/ADT/EquivalenceClasses.h"
+#include "gmock/gmock.h"
#include "gtest/gtest.h"
using namespace llvm;
@@ -66,6 +67,19 @@ TEST(EquivalenceClassesTest, TwoSets) {
EXPECT_FALSE(EqClasses.isEquivalent(i, j));
}
+TEST(EquivalenceClassesTest, MembersIterator) {
+ EquivalenceClasses<int> EC;
+ EC.unionSets(1, 2);
+ EC.insert(4);
+ EC.insert(5);
+ EC.unionSets(5, 1);
+ EXPECT_EQ(EC.getNumClasses(), 2u);
+
+ EquivalenceClasses<int>::iterator I = EC.findValue(EC.getLeaderValue(1));
+ EXPECT_THAT(EC.members(I), testing::ElementsAre(5, 1, 2));
+ EXPECT_EQ(EC.members(EC.end()).begin(), EC.member_end());
+}
+
// Type-parameterized tests: Run the same test cases with different element
// types.
template <typename T> class ParameterizedTest : public testing::Test {};
|
kuhar
approved these changes
Mar 7, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/14679 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/175/builds/14501 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/13399 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/140/builds/18506 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/46/builds/13196 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/133/builds/12458 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/185/builds/14423 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/54/builds/7105 Here is the relevant piece of the build log for the reference |
|
@artagnon Hi, as the llvm-ci bot notes above, a test in this PR is failing. We would like to revert, if this will take some time to fix. We're also seeing this failure in our internal CI. |
vitalybuka
added a commit
that referenced
this pull request
Mar 8, 2025
)" This reverts commit 21d973d.
Please include the original PR. The title says reland, but it's not clear why it was reverted. |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 8, 2025
…elper" (#130380) Reverts llvm/llvm-project#130319 Multiple bot failures.
jph-13
pushed a commit
to jph-13/llvm-project
that referenced
this pull request
Mar 21, 2025
Changes: Fix the expectations in EquivalenceClassesTest.MemberIterator, also fixing a build failure.
artagnon
added a commit
to artagnon/llvm-project
that referenced
this pull request
Apr 4, 2025
Introduce members() iterator-helper to shorten the members_{begin,end}
idiom. A previous attempt of this patch was llvm#130319, which had to be
reverted due to unit-test failures when attempting to call members() on
the end iterator. In this patch, members() accepts either an ECValue or
an ElemTy, which is more inututive and doesn't suffer from the same
issue.
artagnon
added a commit
to artagnon/llvm-project
that referenced
this pull request
Apr 4, 2025
Introduce members() iterator-helper to shorten the members_{begin,end}
idiom. A previous attempt of this patch was llvm#130319, which had to be
reverted due to unit-test failures when attempting to call members() on
the end iterator. In this patch, members() accepts either an ECValue or
an ElemTy, which is more inututive and doesn't suffer from the same
issue.
artagnon
added a commit
that referenced
this pull request
Apr 4, 2025
Introduce members() iterator-helper to shorten the members_{begin,end}
idiom. A previous attempt of this patch was #130319, which had to be
reverted due to unit-test failures when attempting to call members() on
the end iterator. In this patch, members() accepts either an ECValue or
an ElemTy, which is more intuitive and doesn't suffer from the same
issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes: Fix the expectations in EquivalenceClassesTest.MemberIterator, also fixing a build failure.