sammccall added a reviewer: aaron.ballman.
sammccall added a subscriber: aaron.ballman.
sammccall added a comment.
This looks good to me, I think we should ask @aaron.ballman about the memory
increase.
If we wanted to eliminate SLocEntry::Offset then we could add
SourceManager::getOffset(FileID) based on this table and deprecate
SLocEntry::getOffset().
We'd have to tackle loaded SLocs for that though.
> SemaExpr.cpp: 149529494 -> 149502874
Are these numbers backwards? Why would memory usage go down?
================
Comment at: clang/include/clang/Basic/SourceManager.h:697
+ /// FileID lookup.
+ SmallVector<SourceLocation::UIntTy> LocalLocOffsetTable;
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > `LocalSLocEntryTable` is a vec of `SrcMgr::SLocEntry`.
> > `SrcMgr::SLocEntry`'s `Offset` is:
> > ```
> > static constexpr int OffsetBits = 8 * sizeof(SourceLocation::UIntTy) - 1;
> > SourceLocation::UIntTy Offset : OffsetBits;
> > ```
> > So this is a vector of 32b values vs the 24b values we need. Does packing
> > this by declaring a struct improve locality further?
> >
> > ```
> > diff --git a/clang/include/clang/Basic/SourceManager.h
> > b/clang/include/clang/Basic/SourceManager.h
> > index 70ba85bbe948..518c6ed59120 100644
> > --- a/clang/include/clang/Basic/SourceManager.h
> > +++ b/clang/include/clang/Basic/SourceManager.h
> > @@ -692,9 +692,15 @@ class SourceManager : public
> > RefCountedBase<SourceManager> {
> > /// Positive FileIDs are indexes into this table. Entry 0 indicates an
> > invalid
> > /// expansion.
> > SmallVector<SrcMgr::SLocEntry, 0> LocalSLocEntryTable;
> > +
> > + struct SMOffset {
> > + SourceLocation::UIntTy Offset : 24;
> > + SMOffset (SourceLocation::UIntTy O): Offset(O){}
> > + operator SourceLocation::UIntTy() const { return Offset; }
> > + };
> > /// An in-parallel SlocEntry offset table, merely used for speeding up
> > the
> > /// FileID lookup.
> > - SmallVector<SourceLocation::UIntTy> LocalLocOffsetTable;
> > + SmallVector<SMOffset> LocalLocOffsetTable;
> >
> > /// The table of SLocEntries that are loaded from other modules.
> > ///
> > ```
> `struct SMOffset { ... } __attribute__((packed));`
> So this is a vector of 32b values vs the 24b values we need
It's 31 bits: `8 * sizeof - 1`, not `8 * (sizeof - 1)`.
(And none of those bits are spare in practice)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135440/new/
https://reviews.llvm.org/D135440
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
