[ADT] Make set_subtract more efficient when subtrahend is larger (NFC) #98702
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
If the subtrahend is larger, iterate the minuend set instead. Noticed when subtracting a large set from a number of other smaller sets for an upcoming MemProf change, this change makes that much faster. I subsequently found a couple of callsites in one file that were calling set_subtract with a vector subtrahend, which doesn't have the "count()" interface. Add a separate helper for subtracting a vector.
|
@llvm/pr-subscribers-llvm-adt Author: Teresa Johnson (teresajohnson) ChangesIf the subtrahend is larger, iterate the minuend set instead. Noticed when subtracting a large set from a number of other smaller I subsequently found a couple of callsites in one file that were calling Full diff: https://github.com/llvm/llvm-project/pull/98702.diff 2 Files Affected:
diff --git a/llvm/include/llvm/ADT/SetOperations.h b/llvm/include/llvm/ADT/SetOperations.h
index 1a911b239f4c..7be7a55ae13c 100644
--- a/llvm/include/llvm/ADT/SetOperations.h
+++ b/llvm/include/llvm/ADT/SetOperations.h
@@ -92,9 +92,27 @@ S1Ty set_difference(const S1Ty &S1, const S2Ty &S2) {
return Result;
}
+/// set_subtract_vec(A, B) - Compute A := A - B, where B can be a vector.
+///
+template <class S1Ty, class S2Ty>
+void set_subtract_vec(S1Ty &S1, const S2Ty &S2) {
+ for (typename S2Ty::const_iterator SI = S2.begin(), SE = S2.end(); SI != SE;
+ ++SI)
+ S1.erase(*SI);
+}
+
/// set_subtract(A, B) - Compute A := A - B
///
+/// Selects the set to iterate based on the relative sizes of A and B for better
+/// efficiency.
+///
template <class S1Ty, class S2Ty> void set_subtract(S1Ty &S1, const S2Ty &S2) {
+ if (S1.size() < S2.size()) {
+ for (typename S1Ty::iterator SI = S1.begin(), SE = S1.end(); SI != SE; ++SI)
+ if (S2.count(*SI))
+ S1.erase(SI);
+ return;
+ }
for (typename S2Ty::const_iterator SI = S2.begin(), SE = S2.end(); SI != SE;
++SI)
S1.erase(*SI);
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index d0d3af0e5e4f..c80edf974a67 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -2909,7 +2909,8 @@ void MachineVerifier::checkLiveness(const MachineOperand *MO, unsigned MONum) {
void MachineVerifier::visitMachineBundleAfter(const MachineInstr *MI) {
BBInfo &MInfo = MBBInfoMap[MI->getParent()];
set_union(MInfo.regsKilled, regsKilled);
- set_subtract(regsLive, regsKilled); regsKilled.clear();
+ set_subtract_vec(regsLive, regsKilled);
+ regsKilled.clear();
// Kill any masked registers.
while (!regMasks.empty()) {
const uint32_t *Mask = regMasks.pop_back_val();
@@ -2918,7 +2919,8 @@ void MachineVerifier::visitMachineBundleAfter(const MachineInstr *MI) {
MachineOperand::clobbersPhysReg(Mask, Reg.asMCReg()))
regsDead.push_back(Reg);
}
- set_subtract(regsLive, regsDead); regsDead.clear();
+ set_subtract_vec(regsLive, regsDead);
+ regsDead.clear();
set_union(regsLive, regsDefined); regsDefined.clear();
}
|
Comment on lines
110
to
115
| if (S1.size() < S2.size()) { | ||
| for (typename S1Ty::iterator SI = S1.begin(), SE = S1.end(); SI != SE; ++SI) | ||
| if (S2.count(*SI)) | ||
| S1.erase(SI); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I wonder if we could teach set_subtract to use the new path S1.size() < S2.size() when either S2Ty::contains or S2Ty::find is available:
Suggested change
| if (S1.size() < S2.size()) { | |
| for (typename S1Ty::iterator SI = S1.begin(), SE = S1.end(); SI != SE; ++SI) | |
| if (S2.count(*SI)) | |
| S1.erase(SI); | |
| return; | |
| } | |
| using ElemTy = decltype(*S1.begin()); | |
| if constexpr (detail::HasMemberContains<S2Ty, ElemTy> || | |
| detail::HasMemberFind<S2Ty, ElemTy>) { | |
| if (S1.size() < S2.size()) { | |
| for (typename S1Ty::iterator SI = S1.begin(), SE = S1.end(); SI != SE; ++SI) | |
| if (llvm::is_contained(S2, *SI)) | |
| S1.erase(SI); | |
| return; | |
| } | |
| } |
This way, we don't have to have set_subtract_vec. If S2Ty is a vector, the constexpr if would automatically drop the new path. I'm using detail::HasMemberContains<S2Ty, ElemTy> and detail::HasMemberFind<S2Ty, ElemTy> as a heuristic to see if S2Ty offers a sublinear lookup like O(1) or O(log(n)). Calling is_contained without the constexpr if would be a disaster, invoking a linear look up when S2Ty is a vector.
@kuhar Any thoughts here? I recall you generalized is_contained a while ago at 0eaacc2.
There was a problem hiding this comment.
This makes sense to me in the absence of more standardized type traits we could use.
There was a problem hiding this comment.
Great tip! Done.
nikic
reviewed
Jul 15, 2024
| // wouldn't be efficient if it did. In the absence of a more direct check, | ||
| // ensure the type supports the contains or find interfaces. | ||
| if constexpr (detail::HasMemberContains<S2Ty, ElemTy> || | ||
| detail::HasMemberFind<S2Ty, ElemTy>) { |
There was a problem hiding this comment.
I think this should only check for contains, and the count call below should be adjusted to use contains instead to match.
There was a problem hiding this comment.
I think the detail::HasMemberContains and detail::HasMemberFind checks should go with is_contained in place of count below. This way, is_contained will pick C.contains(E) or C.find(E) != C.end() for you depending on the availability.
There was a problem hiding this comment.
I don't think there is a need to support find() here, especially as the rest of SetOperations.h does not support it either.
There was a problem hiding this comment.
I went ahead and changed it to just check and use contains
kuhar
approved these changes
Jul 17, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/177/builds/1789 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/129/builds/2094 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/138/builds/1400 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/53/builds/1591 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/116/builds/1389 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/157/builds/2806 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/153/builds/3312 Here is the relevant piece of the build log for the reference: |
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
#98702) If the subtrahend is larger, iterate the minuend set instead. Noticed when subtracting a large set from a number of other smaller sets for an upcoming MemProf change, this change makes that much faster. I subsequently found a couple of callsites in one file that were calling set_subtract with a vector subtrahend, which doesn't have the "count()" interface. Add a separate helper for subtracting a vector.
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
…ger (NFC)" (#99386) Summary: Reverts #98702 This broke some mlir code and needs investigation. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250971
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.
If the subtrahend is larger, iterate the minuend set instead.
Noticed when subtracting a large set from a number of other smaller
sets for an upcoming MemProf change, this change makes that much faster.
I subsequently found a couple of callsites in one file that were calling
set_subtract with a vector subtrahend, which doesn't have the "count()"
interface. Add a separate helper for subtracting a vector.