Skip to content

Commit

Permalink
[Serialization] No transitive identifier change (llvm#92085)
Browse files Browse the repository at this point in the history
Following of llvm#92083

The motivation is still cutting of the unnecessary change in the
dependency chain. See the above link (recursively) for details.

After this patch, (and the above patch), we can already do something
pretty interesting. For example,

#### Motivation example

```

//--- m-partA.cppm
export module m:partA;

export inline int getA() {
    return 43;
}

export class A {
public:
    int getMem();
};

export template <typename T>
class ATempl {
public:
    T getT();
};

//--- m-partA.v1.cppm
export module m:partA;

export inline int getA() {
    return 43;
}

// Now we add a new declaration without introducing a new type.
// The consuming module which didn't use m:partA completely is expected to be
// not changed.
export inline int getA2() {
    return 88;
}

export class A {
public:
    int getMem();
    // Now we add a new declaration without introducing a new type.
    // The consuming module which didn't use m:partA completely is expected to be
    // not changed.
    int getMem2();
};

export template <typename T>
class ATempl {
public:
    T getT();
    // Add a new declaration without introducing a new type.
    T getT2();
};

//--- m-partB.cppm
export module m:partB;

export inline int getB() {
    return 430;
}

//--- m.cppm
export module m;
export import :partA;
export import :partB;

//--- useBOnly.cppm
export module useBOnly;
import m;

export inline int get() {
    return getB();
}
```

In this example, module `m` exports two partitions `:partA` and
`:partB`. And a consumer `useBOnly` only consumes the entities from
`:partB`. So we don't hope the BMI of `useBOnly` changes if only
`:partA` changes. After this patch, we can make it if the change of
`:partA` doesn't introduce new types. (And we can get rid of this if we
make no-transitive-type-change).

As the example shows, when we change the implementation of `:partA` from
`m-partA.cppm` to `m-partA.v1.cppm`, we add new function declaration
`getA2()` at the global namespace, add a new member function `getMem2()`
to class `A` and add a new member function to `getT2()` to class
template `ATempl`. And since `:partA` is not used by `useBOnly`
completely, the BMI of `useBOnly` won't change after we made above
changes.

#### Design details

Method used in this patch is similar with
llvm#92083 and
llvm#86912. It extends the 32 bit
IdentifierID to 64 bits and use the higher 32 bits to store the module
file index. So that the encoding of the identifier won't get affected by
other modules.

#### Overhead

Similar with llvm#92083 and
llvm#86912. The change is only
expected to increase the size of the on-disk .pcm files and not affect
the compile-time performances. And from my experiment, the size of the
on-disk change only increase 1%+ and observe no compile-time impacts.

#### Future Plans

I'll try to do the same thing for type ids. IIRC, it won't change the
dependency graph if we add a new type in an unused units. I do think
this is a significant win. And this will be a pretty good answer to "why
modules are better than headers."
  • Loading branch information
ChuanqiXu9 authored and AlexisPerry committed Jun 27, 2024
1 parent a375b01 commit 6cb9d57
Show file tree
Hide file tree
Showing 9 changed files with 210 additions and 82 deletions.
2 changes: 1 addition & 1 deletion clang/include/clang/Lex/ExternalPreprocessorSource.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class ExternalPreprocessorSource {
/// Return the identifier associated with the given ID number.
///
/// The ID 0 is associated with the NULL identifier.
virtual IdentifierInfo *GetIdentifier(unsigned ID) = 0;
virtual IdentifierInfo *GetIdentifier(uint64_t ID) = 0;

/// Map a module ID to a module.
virtual Module *getModule(unsigned ModuleID) = 0;
Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/Serialization/ASTBitCodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ const unsigned VERSION_MINOR = 1;
///
/// The ID numbers of identifiers are consecutive (in order of discovery)
/// and start at 1. 0 is reserved for NULL.
using IdentifierID = uint32_t;
using IdentifierID = uint64_t;

/// The number of predefined identifier IDs.
const unsigned int NUM_PREDEF_IDENT_IDS = 1;
Expand Down
19 changes: 8 additions & 11 deletions clang/include/clang/Serialization/ASTReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -661,14 +661,6 @@ class ASTReader
/// been loaded.
std::vector<IdentifierInfo *> IdentifiersLoaded;

using GlobalIdentifierMapType =
ContinuousRangeMap<serialization::IdentifierID, ModuleFile *, 4>;

/// Mapping from global identifier IDs to the module in which the
/// identifier resides along with the offset that should be added to the
/// global identifier ID to produce a local ID.
GlobalIdentifierMapType GlobalIdentifierMap;

/// A vector containing macros that have already been
/// loaded.
///
Expand Down Expand Up @@ -1547,6 +1539,11 @@ class ASTReader
/// Translate a \param GlobalDeclID to the index of DeclsLoaded array.
unsigned translateGlobalDeclIDToIndex(GlobalDeclID ID) const;

/// Translate an \param IdentifierID ID to the index of IdentifiersLoaded
/// array and the corresponding module file.
std::pair<ModuleFile *, unsigned>
translateIdentifierIDToIndex(serialization::IdentifierID ID) const;

public:
/// Load the AST file and validate its contents against the given
/// Preprocessor.
Expand Down Expand Up @@ -2131,7 +2128,7 @@ class ASTReader
/// Load a selector from disk, registering its ID if it exists.
void LoadSelector(Selector Sel);

void SetIdentifierInfo(unsigned ID, IdentifierInfo *II);
void SetIdentifierInfo(serialization::IdentifierID ID, IdentifierInfo *II);
void SetGloballyVisibleDecls(IdentifierInfo *II,
const SmallVectorImpl<GlobalDeclID> &DeclIDs,
SmallVectorImpl<Decl *> *Decls = nullptr);
Expand All @@ -2158,10 +2155,10 @@ class ASTReader
return DecodeIdentifierInfo(ID);
}

IdentifierInfo *getLocalIdentifier(ModuleFile &M, unsigned LocalID);
IdentifierInfo *getLocalIdentifier(ModuleFile &M, uint64_t LocalID);

serialization::IdentifierID getGlobalIdentifierID(ModuleFile &M,
unsigned LocalID);
uint64_t LocalID);

void resolvePendingMacro(IdentifierInfo *II, const PendingMacroInfo &PMInfo);

Expand Down
3 changes: 0 additions & 3 deletions clang/include/clang/Serialization/ModuleFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,6 @@ class ModuleFile {
/// Base identifier ID for identifiers local to this module.
serialization::IdentifierID BaseIdentifierID = 0;

/// Remapping table for identifier IDs in this module.
ContinuousRangeMap<uint32_t, int, 2> IdentifierRemap;

/// Actual data for the on-disk hash table of identifiers.
///
/// This pointer points into a memory buffer, where the on-disk hash
Expand Down
96 changes: 51 additions & 45 deletions clang/lib/Serialization/ASTReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,7 @@ ASTSelectorLookupTrait::ReadKey(const unsigned char* d, unsigned) {
SelectorTable &SelTable = Reader.getContext().Selectors;
unsigned N = endian::readNext<uint16_t, llvm::endianness::little>(d);
const IdentifierInfo *FirstII = Reader.getLocalIdentifier(
F, endian::readNext<uint32_t, llvm::endianness::little>(d));
F, endian::readNext<IdentifierID, llvm::endianness::little>(d));
if (N == 0)
return SelTable.getNullarySelector(FirstII);
else if (N == 1)
Expand All @@ -964,7 +964,7 @@ ASTSelectorLookupTrait::ReadKey(const unsigned char* d, unsigned) {
Args.push_back(FirstII);
for (unsigned I = 1; I != N; ++I)
Args.push_back(Reader.getLocalIdentifier(
F, endian::readNext<uint32_t, llvm::endianness::little>(d)));
F, endian::readNext<IdentifierID, llvm::endianness::little>(d)));

return SelTable.getSelector(N, Args.data());
}
Expand Down Expand Up @@ -1047,7 +1047,8 @@ static bool readBit(unsigned &Bits) {
IdentifierID ASTIdentifierLookupTrait::ReadIdentifierID(const unsigned char *d) {
using namespace llvm::support;

unsigned RawID = endian::readNext<uint32_t, llvm::endianness::little>(d);
IdentifierID RawID =
endian::readNext<IdentifierID, llvm::endianness::little>(d);
return Reader.getGlobalIdentifierID(F, RawID >> 1);
}

Expand All @@ -1065,9 +1066,12 @@ IdentifierInfo *ASTIdentifierLookupTrait::ReadData(const internal_key_type& k,
unsigned DataLen) {
using namespace llvm::support;

unsigned RawID = endian::readNext<uint32_t, llvm::endianness::little>(d);
IdentifierID RawID =
endian::readNext<IdentifierID, llvm::endianness::little>(d);
bool IsInteresting = RawID & 0x01;

DataLen -= sizeof(IdentifierID);

// Wipe out the "is interesting" bit.
RawID = RawID >> 1;

Expand Down Expand Up @@ -1098,7 +1102,7 @@ IdentifierInfo *ASTIdentifierLookupTrait::ReadData(const internal_key_type& k,
bool HadMacroDefinition = readBit(Bits);

assert(Bits == 0 && "Extra bits in the identifier?");
DataLen -= 8;
DataLen -= sizeof(uint16_t) * 2;

// Set or check the various bits in the IdentifierInfo structure.
// Token IDs are read-only.
Expand Down Expand Up @@ -1225,7 +1229,7 @@ ASTDeclContextNameLookupTrait::ReadKey(const unsigned char *d, unsigned) {
case DeclarationName::CXXLiteralOperatorName:
case DeclarationName::CXXDeductionGuideName:
Data = (uint64_t)Reader.getLocalIdentifier(
F, endian::readNext<uint32_t, llvm::endianness::little>(d));
F, endian::readNext<IdentifierID, llvm::endianness::little>(d));
break;
case DeclarationName::ObjCZeroArgSelector:
case DeclarationName::ObjCOneArgSelector:
Expand Down Expand Up @@ -2093,7 +2097,7 @@ HeaderFileInfoTrait::ReadData(internal_key_ref key, const unsigned char *d,
HFI.DirInfo = (Flags >> 1) & 0x07;
HFI.IndexHeaderMapHeader = Flags & 0x01;
HFI.LazyControllingMacro = Reader.getGlobalIdentifierID(
M, endian::readNext<uint32_t, llvm::endianness::little>(d));
M, endian::readNext<IdentifierID, llvm::endianness::little>(d));
if (unsigned FrameworkOffset =
endian::readNext<uint32_t, llvm::endianness::little>(d)) {
// The framework offset is 1 greater than the actual offset,
Expand Down Expand Up @@ -3467,24 +3471,11 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
"duplicate IDENTIFIER_OFFSET record in AST file");
F.IdentifierOffsets = (const uint32_t *)Blob.data();
F.LocalNumIdentifiers = Record[0];
unsigned LocalBaseIdentifierID = Record[1];
F.BaseIdentifierID = getTotalNumIdentifiers();

if (F.LocalNumIdentifiers > 0) {
// Introduce the global -> local mapping for identifiers within this
// module.
GlobalIdentifierMap.insert(std::make_pair(getTotalNumIdentifiers() + 1,
&F));

// Introduce the local -> global mapping for identifiers within this
// module.
F.IdentifierRemap.insertOrReplace(
std::make_pair(LocalBaseIdentifierID,
F.BaseIdentifierID - LocalBaseIdentifierID));

if (F.LocalNumIdentifiers > 0)
IdentifiersLoaded.resize(IdentifiersLoaded.size()
+ F.LocalNumIdentifiers);
}
break;
}

Expand Down Expand Up @@ -4089,7 +4080,6 @@ void ASTReader::ReadModuleOffsetMap(ModuleFile &F) const {
F.ModuleOffsetMap = StringRef();

using RemapBuilder = ContinuousRangeMap<uint32_t, int, 2>::Builder;
RemapBuilder IdentifierRemap(F.IdentifierRemap);
RemapBuilder MacroRemap(F.MacroRemap);
RemapBuilder PreprocessedEntityRemap(F.PreprocessedEntityRemap);
RemapBuilder SubmoduleRemap(F.SubmoduleRemap);
Expand Down Expand Up @@ -4122,8 +4112,6 @@ void ASTReader::ReadModuleOffsetMap(ModuleFile &F) const {

ImportedModuleVector.push_back(OM);

uint32_t IdentifierIDOffset =
endian::readNext<uint32_t, llvm::endianness::little>(Data);
uint32_t MacroIDOffset =
endian::readNext<uint32_t, llvm::endianness::little>(Data);
uint32_t PreprocessedEntityIDOffset =
Expand All @@ -4143,7 +4131,6 @@ void ASTReader::ReadModuleOffsetMap(ModuleFile &F) const {
static_cast<int>(BaseOffset - Offset)));
};

mapOffset(IdentifierIDOffset, OM->BaseIdentifierID, IdentifierRemap);
mapOffset(MacroIDOffset, OM->BaseMacroID, MacroRemap);
mapOffset(PreprocessedEntityIDOffset, OM->BasePreprocessedEntityID,
PreprocessedEntityRemap);
Expand Down Expand Up @@ -8238,7 +8225,6 @@ LLVM_DUMP_METHOD void ASTReader::dump() {
dumpModuleIDMap("Global bit offset map", GlobalBitOffsetsMap);
dumpModuleIDMap("Global source location entry map", GlobalSLocEntryMap);
dumpModuleIDMap("Global type map", GlobalTypeMap);
dumpModuleIDMap("Global identifier map", GlobalIdentifierMap);
dumpModuleIDMap("Global macro map", GlobalMacroMap);
dumpModuleIDMap("Global submodule map", GlobalSubmoduleMap);
dumpModuleIDMap("Global selector map", GlobalSelectorMap);
Expand Down Expand Up @@ -8878,8 +8864,9 @@ void ASTReader::LoadSelector(Selector Sel) {

void ASTReader::SetIdentifierInfo(IdentifierID ID, IdentifierInfo *II) {
assert(ID && "Non-zero identifier ID required");
assert(ID <= IdentifiersLoaded.size() && "identifier ID out of range");
IdentifiersLoaded[ID - 1] = II;
unsigned Index = translateIdentifierIDToIndex(ID).second;
assert(Index < IdentifiersLoaded.size() && "identifier ID out of range");
IdentifiersLoaded[Index] = II;
if (DeserializationListener)
DeserializationListener->IdentifierRead(ID, II);
}
Expand Down Expand Up @@ -8932,6 +8919,22 @@ void ASTReader::SetGloballyVisibleDecls(
}
}

std::pair<ModuleFile *, unsigned>
ASTReader::translateIdentifierIDToIndex(IdentifierID ID) const {
if (ID == 0)
return {nullptr, 0};

unsigned ModuleFileIndex = ID >> 32;
unsigned LocalID = ID & llvm::maskTrailingOnes<IdentifierID>(32);

assert(ModuleFileIndex && "not translating loaded IdentifierID?");
assert(getModuleManager().size() > ModuleFileIndex - 1);

ModuleFile &MF = getModuleManager()[ModuleFileIndex - 1];
assert(LocalID < MF.LocalNumIdentifiers);
return {&MF, MF.BaseIdentifierID + LocalID};
}

IdentifierInfo *ASTReader::DecodeIdentifierInfo(IdentifierID ID) {
if (ID == 0)
return nullptr;
Expand All @@ -8941,45 +8944,48 @@ IdentifierInfo *ASTReader::DecodeIdentifierInfo(IdentifierID ID) {
return nullptr;
}

ID -= 1;
if (!IdentifiersLoaded[ID]) {
GlobalIdentifierMapType::iterator I = GlobalIdentifierMap.find(ID + 1);
assert(I != GlobalIdentifierMap.end() && "Corrupted global identifier map");
ModuleFile *M = I->second;
unsigned Index = ID - M->BaseIdentifierID;
auto [M, Index] = translateIdentifierIDToIndex(ID);
if (!IdentifiersLoaded[Index]) {
assert(M != nullptr && "Untranslated Identifier ID?");
assert(Index >= M->BaseIdentifierID);
unsigned LocalIndex = Index - M->BaseIdentifierID;
const unsigned char *Data =
M->IdentifierTableData + M->IdentifierOffsets[Index];
M->IdentifierTableData + M->IdentifierOffsets[LocalIndex];

ASTIdentifierLookupTrait Trait(*this, *M);
auto KeyDataLen = Trait.ReadKeyDataLength(Data);
auto Key = Trait.ReadKey(Data, KeyDataLen.first);
auto &II = PP.getIdentifierTable().get(Key);
IdentifiersLoaded[ID] = &II;
IdentifiersLoaded[Index] = &II;
markIdentifierFromAST(*this, II);
if (DeserializationListener)
DeserializationListener->IdentifierRead(ID + 1, &II);
DeserializationListener->IdentifierRead(ID, &II);
}

return IdentifiersLoaded[ID];
return IdentifiersLoaded[Index];
}

IdentifierInfo *ASTReader::getLocalIdentifier(ModuleFile &M, unsigned LocalID) {
IdentifierInfo *ASTReader::getLocalIdentifier(ModuleFile &M, uint64_t LocalID) {
return DecodeIdentifierInfo(getGlobalIdentifierID(M, LocalID));
}

IdentifierID ASTReader::getGlobalIdentifierID(ModuleFile &M, unsigned LocalID) {
IdentifierID ASTReader::getGlobalIdentifierID(ModuleFile &M, uint64_t LocalID) {
if (LocalID < NUM_PREDEF_IDENT_IDS)
return LocalID;

if (!M.ModuleOffsetMap.empty())
ReadModuleOffsetMap(M);

ContinuousRangeMap<uint32_t, int, 2>::iterator I
= M.IdentifierRemap.find(LocalID - NUM_PREDEF_IDENT_IDS);
assert(I != M.IdentifierRemap.end()
&& "Invalid index into identifier index remap");
unsigned ModuleFileIndex = LocalID >> 32;
LocalID &= llvm::maskTrailingOnes<IdentifierID>(32);
ModuleFile *MF =
ModuleFileIndex ? M.TransitiveImports[ModuleFileIndex - 1] : &M;
assert(MF && "malformed identifier ID encoding?");

return LocalID + I->second;
if (!ModuleFileIndex)
LocalID -= NUM_PREDEF_IDENT_IDS;

return ((IdentifierID)(MF->Index + 1) << 32) | LocalID;
}

MacroInfo *ASTReader::getMacro(MacroID ID) {
Expand Down
Loading

0 comments on commit 6cb9d57

Please sign in to comment.