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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/include/clang/Basic/SourceManager.h:696
+  /// An in-parallel SlocEntry offset table, merely used for speeding up the
+  /// FileID lookup.
+  SmallVector<SourceLocation::UIntTy> LocalLocOffsetTable;
----------------
Add to this comment that the size is expected to stay in sync with `LocalSLocEntryTable`.


================
Comment at: clang/include/clang/Basic/SourceManager.h:697
+  /// FileID lookup.
+  SmallVector<SourceLocation::UIntTy> LocalLocOffsetTable;
 
----------------
`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.
   ///
```


================
Comment at: clang/lib/Basic/SourceManager.cpp:674
   LocalSLocEntryTable.push_back(SLocEntry::get(NextLocalOffset, Info));
+  LocalLocOffsetTable.push_back(NextLocalOffset);
   assert(NextLocalOffset + Length + 1 > NextLocalOffset &&
----------------
should we add some asserts that `LocalSLocEntryTable.size() == LocalLocOffsetTable.size()` somewhere?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135440/new/

https://reviews.llvm.org/D135440