Skip to content

[NFCI][WPD]Use unique string saver to store type id #106932

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 8 commits into from
Nov 21, 2024
Merged

[NFCI][WPD]Use unique string saver to store type id #106932

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
006c9bf
[NFCI][WPD]Use unique string saver to store type id
mingmingl-llvm Sep 2, 2024
0d5d771
Share string between TypeIdMap and TypeIdCompatibleVTableMap
mingmingl-llvm Nov 19, 2024
e44d1d4
run 'git merge main'
mingmingl-llvm Nov 20, 2024
c8e3dbd
update index yaml parser
mingmingl-llvm Nov 20, 2024
140cf4a
undo format
mingmingl-llvm Nov 20, 2024
87ab8d0
Initialize string savers before its users fwiw
mingmingl-llvm Nov 20, 2024
ebea1fa
move the map value
mingmingl-llvm Nov 20, 2024
a046860
fix clang format
mingmingl-llvm Nov 20, 2024
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
17 changes: 10 additions & 7 deletions llvm/include/llvm/IR/ModuleSummaryIndex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1315,7 +1315,7 @@ using GVSummaryPtrSet = std::unordered_set<GlobalValueSummary *>;
/// Map of a type GUID to type id string and summary (multimap used
/// in case of GUID conflicts).
using TypeIdSummaryMapTy =
std::multimap<GlobalValue::GUID, std::pair<std::string, TypeIdSummary>>;
std::multimap<GlobalValue::GUID, std::pair<StringRef, TypeIdSummary>>;

/// The following data structures summarize type metadata information.
/// For type metadata overview see https://llvm.org/docs/TypeMetadata.html.
Expand Down Expand Up @@ -1351,6 +1351,9 @@ class ModuleSummaryIndex {
/// Holds strings for combined index, mapping to the corresponding module ID.
ModulePathStringTableTy ModulePathStringTable;

BumpPtrAllocator TypeIdSaverAlloc;
UniqueStringSaver TypeIdSaver;

/// Mapping from type identifier GUIDs to type identifier and its summary
/// information. Produced by thin link.
TypeIdSummaryMapTy TypeIdMap;
Expand All @@ -1359,7 +1362,7 @@ class ModuleSummaryIndex {
/// with that type identifier's metadata. Produced by per module summary
/// analysis and consumed by thin link. For more information, see description
/// above where TypeIdCompatibleVtableInfo is defined.
std::map<std::string, TypeIdCompatibleVtableInfo, std::less<>>
std::map<StringRef, TypeIdCompatibleVtableInfo, std::less<>>
TypeIdCompatibleVtableMap;

/// Mapping from original ID to GUID. If original ID can map to multiple
Expand Down Expand Up @@ -1455,8 +1458,8 @@ class ModuleSummaryIndex {
// See HaveGVs variable comment.
ModuleSummaryIndex(bool HaveGVs, bool EnableSplitLTOUnit = false,
bool UnifiedLTO = false)
: HaveGVs(HaveGVs), EnableSplitLTOUnit(EnableSplitLTOUnit),
UnifiedLTO(UnifiedLTO), Saver(Alloc) {}
: TypeIdSaver(TypeIdSaverAlloc), HaveGVs(HaveGVs), EnableSplitLTOUnit(EnableSplitLTOUnit),
UnifiedLTO(UnifiedLTO), Saver(Alloc) {}

// Current version for the module summary in bitcode files.
// The BitcodeSummaryVersion should be bumped whenever we introduce changes
Expand Down Expand Up @@ -1829,8 +1832,8 @@ class ModuleSummaryIndex {
for (auto &[GUID, TypeIdPair] : make_range(TidIter))
if (TypeIdPair.first == TypeId)
return TypeIdPair.second;
auto It = TypeIdMap.insert(
{GlobalValue::getGUID(TypeId), {std::string(TypeId), TypeIdSummary()}});
auto It = TypeIdMap.insert({GlobalValue::getGUID(TypeId),
{TypeIdSaver.save(TypeId), TypeIdSummary()}});
return It->second.second;
}

Expand Down Expand Up @@ -1859,7 +1862,7 @@ class ModuleSummaryIndex {
/// the ThinLTO backends.
TypeIdCompatibleVtableInfo &
getOrInsertTypeIdCompatibleVtableSummary(StringRef TypeId) {
return TypeIdCompatibleVtableMap[std::string(TypeId)];
return TypeIdCompatibleVtableMap[TypeIdSaver.save(TypeId)];
}

/// For the given \p TypeId, this returns the TypeIdCompatibleVtableMap
Expand Down
19 changes: 16 additions & 3 deletions llvm/include/llvm/IR/ModuleSummaryIndexYAML.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,11 +313,11 @@ template <> struct CustomMappingTraits<TypeIdSummaryMapTy> {
static void inputOne(IO &io, StringRef Key, TypeIdSummaryMapTy &V) {
TypeIdSummary TId;
io.mapRequired(Key.str().c_str(), TId);
V.insert({GlobalValue::getGUID(Key), {std::string(Key), TId}});
V.insert({GlobalValue::getGUID(Key), {Key, TId}});
}
static void output(IO &io, TypeIdSummaryMapTy &V) {
for (auto &TidIter : V)
io.mapRequired(TidIter.second.first.c_str(), TidIter.second.second);
io.mapRequired(TidIter.second.first.str().c_str(), TidIter.second.second);
}
};

Expand All @@ -327,7 +327,20 @@ template <> struct MappingTraits<ModuleSummaryIndex> {
if (!io.outputting())
CustomMappingTraits<GlobalValueSummaryMapTy>::fixAliaseeLinks(
index.GlobalValueMap);
io.mapOptional("TypeIdMap", index.TypeIdMap);

if (io.outputting()) {
io.mapOptional("TypeIdMap", index.TypeIdMap);
} else {
TypeIdSummaryMapTy TypeIdMap;
io.mapOptional("TypeIdMap", TypeIdMap);
for (auto &[TypeGUID, TypeIdSummaryMap] : TypeIdMap) {
// Save type id references in index and point TypeIdMap to use the
// references owned by index.
StringRef KeyRef = index.TypeIdSaver.save(TypeIdSummaryMap.first);
index.TypeIdMap.insert({TypeGUID, {KeyRef, TypeIdSummaryMap.second}});
Copy link
Contributor

@teresajohnson teresajohnson Nov 20, 2024

Choose a reason for hiding this comment

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

Instead of doing all this can you instead change CustomMappingTraits to invoke index.getOrInsertTypeIdSummary?

Copy link
Contributor Author

@mingmingl-llvm mingmingl-llvm Nov 20, 2024

Choose a reason for hiding this comment

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

Before I figured out this way, I tried to change CustomMappingTraits. If I read correctly, the input/output methods don't take 'index' as a parameter, and 'TypeIdMap' doesn't have a pointer to 'index' either.

I updated the PR to 'move' TypeIdSummaryMap.second in the latest reply. yaml IO is likely not on the critical path, but std::move should be safe to invoke here.

Copy link
Contributor

@teresajohnson teresajohnson Nov 20, 2024

Choose a reason for hiding this comment

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

ah. ok yeah I missed the fact that you don't have the index there.

}
}

io.mapOptional("WithGlobalValueDeadStripping",
index.WithGlobalValueDeadStripping);

Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4165,7 +4165,7 @@ static void writeWholeProgramDevirtResolution(

static void writeTypeIdSummaryRecord(SmallVector<uint64_t, 64> &NameVals,
StringTableBuilder &StrtabBuilder,
const std::string &Id,
StringRef Id,
const TypeIdSummary &Summary) {
NameVals.push_back(StrtabBuilder.add(Id));
NameVals.push_back(Id.size());
Expand All @@ -4184,7 +4184,7 @@ static void writeTypeIdSummaryRecord(SmallVector<uint64_t, 64> &NameVals,

static void writeTypeIdCompatibleVtableSummaryRecord(
SmallVector<uint64_t, 64> &NameVals, StringTableBuilder &StrtabBuilder,
const std::string &Id, const TypeIdCompatibleVtableInfo &Summary,
StringRef Id, const TypeIdCompatibleVtableInfo &Summary,
ValueEnumerator &VE) {
NameVals.push_back(StrtabBuilder.add(Id));
NameVals.push_back(Id.size());
Expand Down
Loading