dexonsmith added a comment.
Implementation looks a lot cleaner!
I'd still like to drop NumIncludes first if possible because I think it'll be
easier to reason about without this extra layer of complexity. Also, that'd
mitigate the potential regression in `.pcm` size.
(Note: I'd be more comfortable with @vsapsai and/or @rsmith reviewing the
overall direction; I just jumped in for the implementation details.)
================
Comment at: clang/include/clang/Lex/HeaderSearch.h:133-139
-
- /// Determine whether this is a non-default header file info, e.g.,
- /// it corresponds to an actual header we've included or tried to include.
- bool isNonDefault() const {
- return isImport || isPragmaOnce || NumIncludes || ControllingMacro ||
- ControllingMacroID;
- }
----------------
Looks like this is already dead code? If so, please separate out and commit
ahead of time (e.g., now).
================
Comment at: clang/include/clang/Serialization/ModuleFile.h:399
+ /// include information.
+ llvm::StringMap<llvm::SmallVector<uint64_t, 64>> SubmoduleIncludedFiles;
+
----------------
Each StringMapEntry is going to have a pretty big allocation here, for a 512B
vector. Given that it doesn't need to be after creation, it'd be more efficient
to use this pattern:
```
lang=c++
llvm::StringMap<ArrayRef<uint64_t>> SubmoduleIncludedFiles;
SpecificBumpPtrAlloc<uint64_t> SubmoduleIncludedFilesAlloc;
// later
MutableArrayRef<uint64_t> Files(SubmoduleIncludedFiles.Allocate(Record.size()),
Record.size());
llvm::copy(Record, Files.begin());
SubmoduleIncludedFiles[Key] = Files;
```
That said, I feel like this should just be:
```
lang=c++
DenseMap<StringRef, StringRef> SubmoduleIncludedFiles;
```
The key can point at the name of the submodule, which must already exist
somewhere without needing a StringMap to create a new copy of it. The value is
a lazily deserialized blob.
================
Comment at: clang/lib/Lex/Preprocessor.cpp:1323
+ auto &ModNumIncludes = SubmoduleIncludeStates[M].NumIncludes;
+ for (unsigned UID = 0; UID <= ModNumIncludes.maxUID(); ++UID) {
+ CurSubmoduleIncludeState->NumIncludes[UID] += ModNumIncludes[UID];
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > jansvoboda11 wrote:
> > > Iterating over all FileEntries is probably not very efficient, as
> > > Volodymyr mentioned. Thinking about how to make this more efficient...
> > My suggestion above to drop FileEntryMap in favour of a simple DenseMap
> > would help a bit, just iterating through the files actually included by the
> > submodules.
> >
> > Further, I wonder if "num-includes"/file/submodule (`unsigned`) is actually
> > useful, vs. "was-included"/file/submodule (`bool`). The only observer I see
> > is `HeaderSearch::PrintStats()` but maybe I missed something?
> >
> > If I'm right and we can switch to `bool`, then NumIncludes becomes a
> > `DenseSet<FileEntry *> IncludedFiles` (or `DenseSet<unsigned>` for UIDs).
> > (BTW, I also wondered if you could rotate the map, using File as the outer
> > key, and then use bitsets for the sbumodules, but I doubt this is better,
> > since most files are only included by a few submodules, right?)
> >
> > Then you can just do a set union here. Also simplifies bitcode
> > serialization.
> >
> > (If a `bool`/set is sufficient, then I'd suggest landing that
> > first/separately, before adding the per-submodule granularity in this
> > patch.)
> For each file, we need to have three distinct states: not included at all,
> included exactly once (`firstTimeLexingFile`), included more than once. This
> means we can't use a single `DenseSet`.
>
> But we could use a `DenseMap<Key, bool>`, where "not included at all" can be
> expressed as being absent from the map, exactly once as having `true` in the
> map and more than once as having `false` in the map. Alternatively, we could
> use two `DenseSet` instances to encode the same, but I don't think having two
> lookups per file to determine stuff is efficient.
>
> I can look into this in a follow-up patch.
Seems like a DenseSet could still be used by having HeaderInfo pass back the
WasInserted bit from the insertion to the preprocessor, and threading it
through to Preprocessor::HandleEndOfFile (the only caller of
FirstTimeLexingFile):
```
lang=c++
bool IsFirst = Set.insert(Key).second;
```
The threading doesn't seem too hard. Looking at main:
- Preprocessor::HandleHeaderIncludeOrImport calls
HeaderInfo::ShouldEnterIncludeFile. This does the `++FI.NumIncludes` (going
from 0 to 1). Instead, it could be `IsFirst = !FI.WasIncluded; FI.WasIncluded =
true;`, then return `IsFirst` somehow. (Then your patch can pull `IsFirst` from
the `insert().second`).
- Preprocessor::HandleHeaderIncludeOrImport calls
Preprocessor::EnterSourceFile. This creates a new Lexer for that file.
`IsFirst` can be stored on that Lexer.
- Preprocessor::HandleEndOfFile calls FirstTimeLexingFile. Instead, it can
check the new accessor `CurLexer->isFirstTimeLexing()`.
> I can look into this in a follow-up patch.
Follow-up might be okay, but it'd be nice to remove an axis of complexity
before adding a new one if it's reasonable. E.g., it'll be easier to debug
emergent issues from changing it to a simple set since there's less machinery
to worry about.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:5720-5722
+ case SUBMODULE_NUM_INCLUDES:
+ F.SubmoduleIncludedFiles.insert(
+ {CurrentModule->getFullModuleName(), Record});
----------------
This looks lazy, but a bunch of work was just done to decode the `Record` from
bitcode. To make this actually lazy, you can encode the data in a blob, which
doesn't have to be decoded until it's used.
================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2173-2177
+void ASTWriter::addIncludedFiles(
+ const llvm::DenseMap<const FileEntry *, unsigned> &Files,
+ RecordDataImpl &Record) {
+ // Map the FileEntry map into an input file ID vector.
+ llvm::SmallVector<std::pair<uint64_t, unsigned>> FileIDs;
----------------
Why does the count need to be encoded?
The only observer is `Preprocessor::HandleEndOfFile`. If it gets called again
for this file, it'll be after `++NumIncludes`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112915/new/
https://reviews.llvm.org/D112915
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
