[NFCI][BitcodeReader]Read real GUID from VI as opposed to storing it in map

[mingmingl-llvm]

Currently, ValueIdToValueInfoMap [1] stores std::tuple<ValueInfo, GlobalValue::GUID /* original GUID */, GlobalValue::GUID /* real GUID*/ >. This change updates the stored value type to std::pair<ValueInfo, GlobalValue::GUID /* original GUID */>, and reads real GUID from ValueInfo.

When an entry is inserted into ValueIdToValueInfoMap, ValueInfo is created or inserted using real GUID [2]. ValueInfo keeps a pointer to GlobalValueMap [3], using either GUID or {GUID, Name} [4] when reading per-module summaries to create a combined summary.

[1] owned by per module-summary bitcode reader

llvm-project/llvm/lib/Bitcode/Reader/BitcodeReader.cpp

Lines 947 to 950 in caebb45

	// The third tuple member is the real GUID of the ValueInfo.
	DenseMap<unsigned,
	std::tuple<ValueInfo, GlobalValue::GUID, GlobalValue::GUID>>
	ValueIdToValueInfoMap;

[2] first, second, third
[3]

llvm-project/llvm/include/llvm/IR/ModuleSummaryIndex.h

Lines 1427 to 1431 in caebb45

	GlobalValueSummaryMapTy::value_type *
	getOrInsertValuePtr(GlobalValue::GUID GUID) {
	return &*GlobalValueMap.emplace(GUID, GlobalValueSummaryInfo(HaveGVs))
	.first;
	}

[4]

llvm-project/llvm/include/llvm/IR/ModuleSummaryIndex.h

Line 1631 in caebb45

ValueInfo getOrInsertValueInfo(GlobalValue::GUID GUID, StringRef Name) {

and

llvm-project/llvm/include/llvm/IR/ModuleSummaryIndex.h

Line 1621 in caebb45

ValueInfo getOrInsertValueInfo(GlobalValue::GUID GUID) {

[kazutakahirata]

Nice finding! Let me see if I understand things correctly.

• We always create an instance of ValueInfo with the same GUID as std::get<2>.
• Once an instance of ValueInfo is created, its pointer portion never changes.
• The pointer portion of ValueInfo points to an key-value pair of GlobalValueSummaryMapTy, so the key portion, GUID, shouldn't change.

So, std::get<2>(VIAndOriginalGUID) and VIAndOriginalGUID.first.getGUID() must be always the same.

Did I get this right?

[mingmingl-llvm]

• We always create an instance of ValueInfo with the same GUID as std::get<2>.

Yup this is the case for all (first, second, third) insertions.

Once an instance of ValueInfo is created, its pointer portion never changes.
The pointer portion of ValueInfo points to an key-value pair of GlobalValueSummaryMapTy, so the key portion, GUID, shouldn't change.

This is the case as well.

Basically, ValueIdToValueInfoMap is owned and used by the per module-summary bitcode reader, to associate (serialized, per-module) 'value-id' with the (in-memory) value summary (from combined summary).

So the ValueInfo's pointer portion essentially points to the global-value summary entry, which is owned by CombinedIndex.

Since bitcode readers are (reasonably) destructed after the per-module summary content are merged into combined index, this change doesn't help reduce peak memory usage. This change is not complex anyway, so I just thought about sending it out.

[kazutakahirata]

LGTM. See a comment about "tuple".

[mingmingl-llvm]

thanks for the reviews!