[LowerTypeTests] Optimize buildBitSet #157386
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
Created using spr 1.3.6
|
@llvm/pr-subscribers-llvm-transforms Author: Vitaly Buka (vitalybuka) Changes
The patch maps all offsets to correspondign On one large internal binary, Full diff: https://github.com/llvm/llvm-project/pull/157386.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Transforms/IPO/LowerTypeTests.h b/llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
index 253d13f81857b..a34cbaf72675b 100644
--- a/llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
+++ b/llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
@@ -64,15 +64,12 @@ struct BitSetBuilder {
uint64_t Min = std::numeric_limits<uint64_t>::max();
uint64_t Max = 0;
- BitSetBuilder() = default;
-
- void addOffset(uint64_t Offset) {
- if (Min > Offset)
- Min = Offset;
- if (Max < Offset)
- Max = Offset;
-
- Offsets.push_back(Offset);
+ explicit BitSetBuilder(ArrayRef<uint64_t> Offsets) : Offsets(Offsets) {
+ if (!Offsets.empty()) {
+ auto [MinIt, MaxIt] = std::minmax_element(Offsets.begin(), Offsets.end());
+ Min = *MinIt;
+ Max = *MaxIt;
+ }
}
LLVM_ABI BitSetInfo build();
diff --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
index 821a9d82ddb0d..be6cba38d8b3c 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -573,28 +573,11 @@ class LowerTypeTestsModule {
};
} // end anonymous namespace
-/// Build a bit set for TypeId using the object layouts in
-/// GlobalLayout.
-static BitSetInfo
-buildBitSet(Metadata *TypeId,
- const DenseMap<GlobalTypeMember *, uint64_t> &GlobalLayout) {
- BitSetBuilder BSB;
-
+/// Build a bit set for list of offsets.
+static BitSetInfo buildBitSet(ArrayRef<uint64_t> Offsets) {
// Compute the byte offset of each address associated with this type
// identifier.
- for (const auto &GlobalAndOffset : GlobalLayout) {
- for (MDNode *Type : GlobalAndOffset.first->types()) {
- if (Type->getOperand(1) != TypeId)
- continue;
- uint64_t Offset =
- cast<ConstantInt>(
- cast<ConstantAsMetadata>(Type->getOperand(0))->getValue())
- ->getZExtValue();
- BSB.addOffset(GlobalAndOffset.second + Offset);
- }
- }
-
- return BSB.build();
+ return BitSetBuilder(Offsets).build();
}
/// Build a test that bit BitOffset mod sizeof(Bits)*8 is set in
@@ -1161,21 +1144,47 @@ void LowerTypeTestsModule::importFunction(Function *F,
F->setVisibility(Visibility);
}
-void LowerTypeTestsModule::lowerTypeTestCalls(
- ArrayRef<Metadata *> TypeIds, Constant *CombinedGlobalAddr,
- const DenseMap<GlobalTypeMember *, uint64_t> &GlobalLayout) {
- // For each type identifier in this disjoint set...
+static auto
+buildBitSets(ArrayRef<Metadata *> TypeIds,
+ const DenseMap<GlobalTypeMember *, uint64_t> &GlobalLayout) {
+ DenseMap<Metadata *, SmallVector<uint64_t, 16>> OffsetsByTypeID;
+ // Pre-populate the map with interesting type identifiers.
+ for (Metadata *TypeId : TypeIds)
+ OffsetsByTypeID[TypeId];
+ for (const auto &[Mem, MemOff] : GlobalLayout) {
+ for (MDNode *Type : Mem->types()) {
+ auto It = OffsetsByTypeID.find(Type->getOperand(1));
+ if (It == OffsetsByTypeID.end())
+ continue;
+ uint64_t Offset =
+ cast<ConstantInt>(
+ cast<ConstantAsMetadata>(Type->getOperand(0))->getValue())
+ ->getZExtValue();
+ It->second.push_back(MemOff + Offset);
+ }
+ }
+
+ SmallVector<std::pair<Metadata *, BitSetInfo>> BitSets;
+ BitSets.reserve(TypeIds.size());
for (Metadata *TypeId : TypeIds) {
- // Build the bitset.
- BitSetInfo BSI = buildBitSet(TypeId, GlobalLayout);
+ BitSets.emplace_back(TypeId, buildBitSet(OffsetsByTypeID[TypeId]));
LLVM_DEBUG({
if (auto MDS = dyn_cast<MDString>(TypeId))
dbgs() << MDS->getString() << ": ";
else
dbgs() << "<unnamed>: ";
- BSI.print(dbgs());
+ BitSets.back().second.print(dbgs());
});
+ }
+
+ return BitSets;
+}
+void LowerTypeTestsModule::lowerTypeTestCalls(
+ ArrayRef<Metadata *> TypeIds, Constant *CombinedGlobalAddr,
+ const DenseMap<GlobalTypeMember *, uint64_t> &GlobalLayout) {
+ // For each type identifier in this disjoint set...
+ for (const auto &[TypeId, BSI] : buildBitSets(TypeIds, GlobalLayout)) {
ByteArrayInfo *BAI = nullptr;
TypeIdLowering TIL;
diff --git a/llvm/unittests/Transforms/IPO/LowerTypeTests.cpp b/llvm/unittests/Transforms/IPO/LowerTypeTests.cpp
index fc41b036ffb6c..3d6b7efbccd91 100644
--- a/llvm/unittests/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/unittests/Transforms/IPO/LowerTypeTests.cpp
@@ -51,10 +51,7 @@ TEST(LowerTypeTests, BitSetBuilder) {
};
for (auto &&T : BSBTests) {
- BitSetBuilder BSB;
- for (auto Offset : T.Offsets)
- BSB.addOffset(Offset);
-
+ BitSetBuilder BSB(T.Offsets);
BitSetInfo BSI = BSB.build();
EXPECT_EQ(T.Bits, BSI.Bits);
|
pcc
reviewed
Sep 10, 2025
| buildBitSets(ArrayRef<Metadata *> TypeIds, | ||
| const DenseMap<GlobalTypeMember *, uint64_t> &GlobalLayout) { | ||
| DenseMap<Metadata *, SmallVector<uint64_t, 16>> OffsetsByTypeID; | ||
| // Pre-populate the map with interesting type identifiers. |
There was a problem hiding this comment.
I don't think this is necessary, all types mentioned in GlobalLayout should also be in TypeIds.
There was a problem hiding this comment.
They are not.
In most cases unique counts are 1 vs 2
But there are cases like 11250 vs 886681
There was a problem hiding this comment.
Right, TypeIds only contains types referenced by llvm.type.test.
pcc
approved these changes
Sep 11, 2025
| buildBitSets(ArrayRef<Metadata *> TypeIds, | ||
| const DenseMap<GlobalTypeMember *, uint64_t> &GlobalLayout) { | ||
| DenseMap<Metadata *, SmallVector<uint64_t, 16>> OffsetsByTypeID; | ||
| // Pre-populate the map with interesting type identifiers. |
There was a problem hiding this comment.
Right, TypeIds only contains types referenced by llvm.type.test.
vitalybuka
deleted the
users/vitalybuka/spr/lowertypetests-optimize-buildbitset
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/30872 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
buildBitSethad a loop trough entire GlobalLayout to pickup matching offsets.The patch maps all offsets to correspondign
TypeId, so we pass prepared list of offsets intobuildBitSet.On one large internal binary,
LowerTypeTeststook 58% of ThinLTO link time before the patch.
After the patch just 7% (absolute saving is 200s).