[llvm] [NFCI][BitcodeReader]Read real GUID from VI as opposed to storing it in map (PR #107735)

Mingming Liu via llvm-commits llvm-commits at lists.llvm.org
Sun Sep 8 00:10:44 PDT 2024


minglotus-6 wrote:

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

Yup this is the case for all ([first](https://github.com/llvm/llvm-project/blob/caebb4562ce634a22f7b13480b19cffc2a6a6730/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L7130-L7133), [second](https://github.com/llvm/llvm-project/blob/caebb4562ce634a22f7b13480b19cffc2a6a6730/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L7221-L7222), [third](https://github.com/llvm/llvm-project/blob/caebb4562ce634a22f7b13480b19cffc2a6a6730/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L7622-L7623)) 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](https://github.com/llvm/llvm-project/blob/cf11eb62e1d0fa41f68b4ca3150eac854ac2e991/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L4364), [per-module](https://github.com/llvm/llvm-project/blob/cf11eb62e1d0fa41f68b4ca3150eac854ac2e991/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L259)) '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](https://github.com/llvm/llvm-project/blob/cf11eb62e1d0fa41f68b4ca3150eac854ac2e991/llvm/include/llvm/IR/ModuleSummaryIndex.h#L176), which is owned by [CombinedIndex](https://github.com/llvm/llvm-project/blob/cf11eb62e1d0fa41f68b4ca3150eac854ac2e991/llvm/include/llvm/LTO/LTO.h#L341). 

Since bitcode readers are [(reasonably) destructed](https://github.com/llvm/llvm-project/blob/bc59b638ae35594b55a439a5986519a3f9cc903d/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L8411-L8413) 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.

https://github.com/llvm/llvm-project/pull/107735


More information about the llvm-commits mailing list