[PATCH] D135440: [SourceManager] Speedup getFileIDLocal with a separate Offset Table.

[Sam McCall via Phabricator via cfe-commits]

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