[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Alex Lorenz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG11d612ac99a6: [clang][Preprocessor] Replace the slow 
translateFile call by a new, faster… (authored by arphaman).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79834

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/unittests/Basic/SourceManagerTest.cpp


Index: clang/unittests/Basic/SourceManagerTest.cpp
===
--- clang/unittests/Basic/SourceManagerTest.cpp
+++ clang/unittests/Basic/SourceManagerTest.cpp
@@ -491,6 +491,30 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, 
Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, isMainFile) {
+  const char *Source = "int x;";
+
+  std::unique_ptr Buf =
+  llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *SourceFile =
+  FileMgr.getVirtualFile("mainFile.cpp", Buf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(SourceFile, std::move(Buf));
+
+  std::unique_ptr Buf2 =
+  llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *SecondFile =
+  FileMgr.getVirtualFile("file2.cpp", Buf2->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(SecondFile, std::move(Buf2));
+
+  FileID MainFileID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+  SourceMgr.setMainFileID(MainFileID);
+
+  EXPECT_TRUE(SourceMgr.isMainFile(FileEntryRef("mainFile.cpp", *SourceFile)));
+  EXPECT_TRUE(
+  SourceMgr.isMainFile(FileEntryRef("anotherName.cpp", *SourceFile)));
+  EXPECT_FALSE(SourceMgr.isMainFile(FileEntryRef("mainFile.cpp", 
*SecondFile)));
+}
+
 #endif
 
 } // anonymous namespace
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2054,8 +2054,7 @@
   // some directives (e.g. #endif of a header guard) will never be seen.
   // Since this will lead to confusing errors, avoid the inclusion.
   if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
-  SourceMgr.translateFile(>getFileEntry()) ==
-  SourceMgr.getMainFileID()) {
+  SourceMgr.isMainFile(*File)) {
 Diag(FilenameTok.getLocation(),
  diag::err_pp_including_mainfile_in_preamble);
 return {ImportAction::None};
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -389,6 +389,14 @@
   createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
 }
 
+bool SourceManager::isMainFile(FileEntryRef SourceFile) {
+  assert(MainFileID.isValid() && "expected initialized SourceManager");
+  auto FE = getFileEntryRefForID(MainFileID);
+  if (!FE)
+return false;
+  return FE->getUID() == SourceFile.getUID();
+}
+
 void SourceManager::initializeForReplay(const SourceManager ) {
   assert(MainFileID.isInvalid() && "expected uninitialized SourceManager");
 
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -813,6 +813,11 @@
 MainFileID = FID;
   }
 
+  /// Returns true when the given FileEntry corresponds to the main file.
+  ///
+  /// The main file should be set prior to calling this function.
+  bool isMainFile(FileEntryRef SourceFile);
+
   /// Set the file ID for the precompiled preamble.
   void setPreambleFileID(FileID Preamble) {
 assert(PreambleFileID.isInvalid() && "PreambleFileID already set!");


Index: clang/unittests/Basic/SourceManagerTest.cpp
===
--- clang/unittests/Basic/SourceManagerTest.cpp
+++ clang/unittests/Basic/SourceManagerTest.cpp
@@ -491,6 +491,30 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, isMainFile) {
+  const char *Source = "int x;";
+
+  std::unique_ptr Buf =
+  llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *SourceFile =
+  FileMgr.getVirtualFile("mainFile.cpp", Buf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(SourceFile, std::move(Buf));
+
+  std::unique_ptr Buf2 =
+  llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *SecondFile =
+  FileMgr.getVirtualFile("file2.cpp", Buf2->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(SecondFile, std::move(Buf2));
+
+  FileID MainFileID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+  SourceMgr.setMainFileID(MainFileID);
+
+  EXPECT_TRUE(SourceMgr.isMainFile(FileEntryRef("mainFile.cpp", *SourceFile)));
+  EXPECT_TRUE(
+  SourceMgr.isMainFile(FileEntryRef("anotherName.cpp", 

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.



Comment at: clang/lib/Basic/SourceManager.cpp:397
+return false;
+  return FE->getUID() == SourceFile.getUID();
+}

arphaman wrote:
> jkorous wrote:
> > I don't really understand all the details here but shouldn't we use this 
> > comparison?
> > ```
> > bool operator==(const FileEntryRef , const FileEntryRef )
> > ```
> The `==` comparison can return false for symlinks due to the different name, 
> but we want to return true for symlinks to the main file as well. 
I suggest adding a comment to the code explaining this when you land it, 
something like:

// Compare UIDs directly so that symlinks compare equal.


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

https://reviews.llvm.org/D79834



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Jan Korous via Phabricator via cfe-commits]

jkorous accepted this revision.
jkorous added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks Alex!


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

https://reviews.llvm.org/D79834



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Alex Lorenz via Phabricator via cfe-commits]

arphaman marked an inline comment as done.
arphaman added inline comments.



Comment at: clang/lib/Basic/SourceManager.cpp:397
+return false;
+  return FE->getUID() == SourceFile.getUID();
+}

jkorous wrote:
> I don't really understand all the details here but shouldn't we use this 
> comparison?
> ```
> bool operator==(const FileEntryRef , const FileEntryRef )
> ```
The `==` comparison can return false for symlinks due to the different name, 
but we want to return true for symlinks to the main file as well. 


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

https://reviews.llvm.org/D79834



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 264044.
arphaman added a comment.

Added test and a comment.


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

https://reviews.llvm.org/D79834

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/unittests/Basic/SourceManagerTest.cpp


Index: clang/unittests/Basic/SourceManagerTest.cpp
===
--- clang/unittests/Basic/SourceManagerTest.cpp
+++ clang/unittests/Basic/SourceManagerTest.cpp
@@ -491,6 +491,30 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, 
Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, isMainFile) {
+  const char *Source = "int x;";
+
+  std::unique_ptr Buf =
+  llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *SourceFile =
+  FileMgr.getVirtualFile("mainFile.cpp", Buf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(SourceFile, std::move(Buf));
+
+  std::unique_ptr Buf2 =
+  llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *SecondFile =
+  FileMgr.getVirtualFile("file2.cpp", Buf2->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(SecondFile, std::move(Buf2));
+
+  FileID MainFileID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+  SourceMgr.setMainFileID(MainFileID);
+
+  EXPECT_TRUE(SourceMgr.isMainFile(FileEntryRef("mainFile.cpp", *SourceFile)));
+  EXPECT_TRUE(
+  SourceMgr.isMainFile(FileEntryRef("anotherName.cpp", *SourceFile)));
+  EXPECT_FALSE(SourceMgr.isMainFile(FileEntryRef("mainFile.cpp", 
*SecondFile)));
+}
+
 #endif
 
 } // anonymous namespace
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2054,8 +2054,7 @@
   // some directives (e.g. #endif of a header guard) will never be seen.
   // Since this will lead to confusing errors, avoid the inclusion.
   if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
-  SourceMgr.translateFile(>getFileEntry()) ==
-  SourceMgr.getMainFileID()) {
+  SourceMgr.isMainFile(*File)) {
 Diag(FilenameTok.getLocation(),
  diag::err_pp_including_mainfile_in_preamble);
 return {ImportAction::None};
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -389,6 +389,14 @@
   createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
 }
 
+bool SourceManager::isMainFile(FileEntryRef SourceFile) {
+  assert(MainFileID.isValid() && "expected initialized SourceManager");
+  auto FE = getFileEntryRefForID(MainFileID);
+  if (!FE)
+return false;
+  return FE->getUID() == SourceFile.getUID();
+}
+
 void SourceManager::initializeForReplay(const SourceManager ) {
   assert(MainFileID.isInvalid() && "expected uninitialized SourceManager");
 
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -813,6 +813,11 @@
 MainFileID = FID;
   }
 
+  /// Returns true when the given FileEntry corresponds to the main file.
+  ///
+  /// The main file should be set prior to calling this function.
+  bool isMainFile(FileEntryRef SourceFile);
+
   /// Set the file ID for the precompiled preamble.
   void setPreambleFileID(FileID Preamble) {
 assert(PreambleFileID.isInvalid() && "PreambleFileID already set!");


Index: clang/unittests/Basic/SourceManagerTest.cpp
===
--- clang/unittests/Basic/SourceManagerTest.cpp
+++ clang/unittests/Basic/SourceManagerTest.cpp
@@ -491,6 +491,30 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, isMainFile) {
+  const char *Source = "int x;";
+
+  std::unique_ptr Buf =
+  llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *SourceFile =
+  FileMgr.getVirtualFile("mainFile.cpp", Buf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(SourceFile, std::move(Buf));
+
+  std::unique_ptr Buf2 =
+  llvm::MemoryBuffer::getMemBuffer(Source);
+  const FileEntry *SecondFile =
+  FileMgr.getVirtualFile("file2.cpp", Buf2->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(SecondFile, std::move(Buf2));
+
+  FileID MainFileID = SourceMgr.getOrCreateFileID(SourceFile, SrcMgr::C_User);
+  SourceMgr.setMainFileID(MainFileID);
+
+  EXPECT_TRUE(SourceMgr.isMainFile(FileEntryRef("mainFile.cpp", *SourceFile)));
+  EXPECT_TRUE(
+  SourceMgr.isMainFile(FileEntryRef("anotherName.cpp", *SourceFile)));
+  EXPECT_FALSE(SourceMgr.isMainFile(FileEntryRef("mainFile.cpp", *SecondFile)));
+}
+
 #endif
 
 } // anonymous namespace
Index: 

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Could you please add a test for this method to 
`clang/unittests/Basic/SourceManagerTest.cpp`?




Comment at: clang/include/clang/Basic/SourceManager.h:816
 
+  /// Returns true when the given FileEntry corresponds to the main file.
+  bool isMainFile(FileEntryRef SourceFile);

Please mention the assertion this method makes about the state of the instance 
it's called on.



Comment at: clang/lib/Basic/SourceManager.cpp:397
+return false;
+  return FE->getUID() == SourceFile.getUID();
+}

I don't really understand all the details here but shouldn't we use this 
comparison?
```
bool operator==(const FileEntryRef , const FileEntryRef )
```


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

https://reviews.llvm.org/D79834



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 263847.
arphaman added a comment.

Drop caching, it's not need for the speedup.


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

https://reviews.llvm.org/D79834

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Lex/PPDirectives.cpp


Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2054,8 +2054,7 @@
   // some directives (e.g. #endif of a header guard) will never be seen.
   // Since this will lead to confusing errors, avoid the inclusion.
   if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
-  SourceMgr.translateFile(>getFileEntry()) ==
-  SourceMgr.getMainFileID()) {
+  SourceMgr.isMainFile(*File)) {
 Diag(FilenameTok.getLocation(),
  diag::err_pp_including_mainfile_in_preamble);
 return {ImportAction::None};
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -389,6 +389,14 @@
   createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
 }
 
+bool SourceManager::isMainFile(FileEntryRef SourceFile) {
+  assert(MainFileID.isValid() && "expected initialized SourceManager");
+  auto FE = getFileEntryRefForID(MainFileID);
+  if (!FE)
+return false;
+  return FE->getUID() == SourceFile.getUID();
+}
+
 void SourceManager::initializeForReplay(const SourceManager ) {
   assert(MainFileID.isInvalid() && "expected uninitialized SourceManager");
 
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -813,6 +813,9 @@
 MainFileID = FID;
   }
 
+  /// Returns true when the given FileEntry corresponds to the main file.
+  bool isMainFile(FileEntryRef SourceFile);
+
   /// Set the file ID for the precompiled preamble.
   void setPreambleFileID(FileID Preamble) {
 assert(PreambleFileID.isInvalid() && "PreambleFileID already set!");


Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2054,8 +2054,7 @@
   // some directives (e.g. #endif of a header guard) will never be seen.
   // Since this will lead to confusing errors, avoid the inclusion.
   if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
-  SourceMgr.translateFile(>getFileEntry()) ==
-  SourceMgr.getMainFileID()) {
+  SourceMgr.isMainFile(*File)) {
 Diag(FilenameTok.getLocation(),
  diag::err_pp_including_mainfile_in_preamble);
 return {ImportAction::None};
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -389,6 +389,14 @@
   createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
 }
 
+bool SourceManager::isMainFile(FileEntryRef SourceFile) {
+  assert(MainFileID.isValid() && "expected initialized SourceManager");
+  auto FE = getFileEntryRefForID(MainFileID);
+  if (!FE)
+return false;
+  return FE->getUID() == SourceFile.getUID();
+}
+
 void SourceManager::initializeForReplay(const SourceManager ) {
   assert(MainFileID.isInvalid() && "expected uninitialized SourceManager");
 
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -813,6 +813,9 @@
 MainFileID = FID;
   }
 
+  /// Returns true when the given FileEntry corresponds to the main file.
+  bool isMainFile(FileEntryRef SourceFile);
+
   /// Set the file ID for the precompiled preamble.
   void setPreambleFileID(FileID Preamble) {
 assert(PreambleFileID.isInvalid() && "PreambleFileID already set!");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

@jkorous it looks like dropping caching works too, this achieves similar perf 
results.


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

https://reviews.llvm.org/D79834



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In D79834#2034666 , @jkorous wrote:

> IIUC the issue is that `SourceManager::translateFile()` basically consists of 
> two blocks of code:
>
>   // First, check the main file ID, since it is common to look for a
>   // location in the main file.
>   if (MainFileID.isValid()) {
> bool Invalid = false;
> const SLocEntry  = getSLocEntry(MainFileID, );
> if (Invalid)
>   return FileID();
>   
> if (MainSLoc.isFile()) {
>   const ContentCache *MainContentCache =
>   MainSLoc.getFile().getContentCache();
>   if (MainContentCache && MainContentCache->OrigEntry == SourceFile)
> return MainFileID;
> }
>   }
>   
>
> and
>
> // The location we're looking for isn't in the main file; look
> // through all of the local source locations.
>   ...
>
>
> The comments suggest that the first block is a heuristic related to our case 
> and the second block I would assume being the expensive part. 
> `SourceManager::getFileEntryRefForID` implementation seems similar to the 
> first block.
>
> It makes sense to me to avoid the expensive search. I'm just wondering - how 
> much speedup do we get with caching the value?


Good question, let me check if caching can be avoided.


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

https://reviews.llvm.org/D79834



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

IIUC the issue is that `SourceManager::translateFile()` basically consists of 
two blocks of code:

  // First, check the main file ID, since it is common to look for a
  // location in the main file.
  if (MainFileID.isValid()) {
bool Invalid = false;
const SLocEntry  = getSLocEntry(MainFileID, );
if (Invalid)
  return FileID();
  
if (MainSLoc.isFile()) {
  const ContentCache *MainContentCache =
  MainSLoc.getFile().getContentCache();
  if (MainContentCache && MainContentCache->OrigEntry == SourceFile)
return MainFileID;
}
  }

and

// The location we're looking for isn't in the main file; look
// through all of the local source locations.
  ...

The comments suggest that the first block is a heuristic related to our case 
and the second block I would assume being the expensive part. 
`SourceManager::getFileEntryRefForID` implementation seems similar to the first 
block.

It makes sense to me to avoid the expensive search. I'm just wondering - how 
much speedup do we get with caching the value?


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

https://reviews.llvm.org/D79834



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 263781.
arphaman added a comment.

fix assertion in the unit test.


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

https://reviews.llvm.org/D79834

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Lex/PPDirectives.cpp


Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2054,8 +2054,7 @@
   // some directives (e.g. #endif of a header guard) will never be seen.
   // Since this will lead to confusing errors, avoid the inclusion.
   if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
-  SourceMgr.translateFile(>getFileEntry()) ==
-  SourceMgr.getMainFileID()) {
+  SourceMgr.isMainFile(*File)) {
 Diag(FilenameTok.getLocation(),
  diag::err_pp_including_mainfile_in_preamble);
 return {ImportAction::None};
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -373,6 +373,7 @@
 
 void SourceManager::clearIDTables() {
   MainFileID = FileID();
+  CachedMainFileEntry = Optional>();
   LocalSLocEntryTable.clear();
   LoadedSLocEntryTable.clear();
   SLocEntryLoaded.clear();
@@ -389,6 +390,15 @@
   createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
 }
 
+bool SourceManager::isMainFile(FileEntryRef SourceFile) {
+  assert(MainFileID.isValid() && "expected initialized SourceManager");
+  if (!CachedMainFileEntry)
+*CachedMainFileEntry = getFileEntryRefForID(MainFileID);
+  if (!*CachedMainFileEntry)
+return false;
+  return (*CachedMainFileEntry)->getUID() == SourceFile.getUID();
+}
+
 void SourceManager::initializeForReplay(const SourceManager ) {
   assert(MainFileID.isInvalid() && "expected uninitialized SourceManager");
 
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -35,6 +35,7 @@
 #define LLVM_CLANG_BASIC_SOURCEMANAGER_H
 
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitVector.h"
@@ -59,9 +60,6 @@
 
 class ASTReader;
 class ASTWriter;
-class FileManager;
-class FileEntry;
-class FileEntryRef;
 class LineTableInfo;
 class SourceManager;
 
@@ -706,6 +704,9 @@
   /// The file ID for the main source file of the translation unit.
   FileID MainFileID;
 
+  /// The file entry for the main source file.
+  Optional> CachedMainFileEntry;
+
   /// The file ID for the precompiled preamble there is one.
   FileID PreambleFileID;
 
@@ -813,6 +814,9 @@
 MainFileID = FID;
   }
 
+  /// Returns true when the given FileEntry corresponds to the main file.
+  bool isMainFile(FileEntryRef SourceFile);
+
   /// Set the file ID for the precompiled preamble.
   void setPreambleFileID(FileID Preamble) {
 assert(PreambleFileID.isInvalid() && "PreambleFileID already set!");


Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2054,8 +2054,7 @@
   // some directives (e.g. #endif of a header guard) will never be seen.
   // Since this will lead to confusing errors, avoid the inclusion.
   if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
-  SourceMgr.translateFile(>getFileEntry()) ==
-  SourceMgr.getMainFileID()) {
+  SourceMgr.isMainFile(*File)) {
 Diag(FilenameTok.getLocation(),
  diag::err_pp_including_mainfile_in_preamble);
 return {ImportAction::None};
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -373,6 +373,7 @@
 
 void SourceManager::clearIDTables() {
   MainFileID = FileID();
+  CachedMainFileEntry = Optional>();
   LocalSLocEntryTable.clear();
   LoadedSLocEntryTable.clear();
   SLocEntryLoaded.clear();
@@ -389,6 +390,15 @@
   createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
 }
 
+bool SourceManager::isMainFile(FileEntryRef SourceFile) {
+  assert(MainFileID.isValid() && "expected initialized SourceManager");
+  if (!CachedMainFileEntry)
+*CachedMainFileEntry = getFileEntryRefForID(MainFileID);
+  if (!*CachedMainFileEntry)
+return false;
+  return (*CachedMainFileEntry)->getUID() == SourceFile.getUID();
+}
+
 void SourceManager::initializeForReplay(const SourceManager ) {
   assert(MainFileID.isInvalid() && "expected uninitialized SourceManager");
 
Index: clang/include/clang/Basic/SourceManager.h

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.
arphaman added a reviewer: jkorous.
Herald added subscribers: ributzka, dexonsmith.
Herald added a project: clang.

[clang][Preprocessor] Replace the slow translateFile call by a new, faster 
isMainFile check

  
  The commit 3c28a2dc6bdc331e5a0d8097a5fa59d06682b9d0 introduced the check that 
checks if we're
  trying to re-enter a main file when building a preamble. Unfortunately this 
slowed down the preamble
  compilation by 80-90% in some test cases, as translateFile is really slow. 
This change checks
  to see if the FileEntry is the main file without calling translateFile, but 
by using the new
  isMainFile check instead. This speeds up preamble building by 1.5-2x for 
certain test cases that we have.
  
  rdar://59361291


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79834

Files:
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Lex/PPDirectives.cpp


Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2054,8 +2054,7 @@
   // some directives (e.g. #endif of a header guard) will never be seen.
   // Since this will lead to confusing errors, avoid the inclusion.
   if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
-  SourceMgr.translateFile(>getFileEntry()) ==
-  SourceMgr.getMainFileID()) {
+  SourceMgr.isMainFile(*File)) {
 Diag(FilenameTok.getLocation(),
  diag::err_pp_including_mainfile_in_preamble);
 return {ImportAction::None};
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -373,6 +373,7 @@
 
 void SourceManager::clearIDTables() {
   MainFileID = FileID();
+  CachedMainFileEntry = None;
   LocalSLocEntryTable.clear();
   LoadedSLocEntryTable.clear();
   SLocEntryLoaded.clear();
@@ -389,6 +390,15 @@
   createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
 }
 
+bool SourceManager::isMainFile(FileEntryRef SourceFile) {
+  assert(MainFileID.isValid() && "expected initialized SourceManager");
+  if (!CachedMainFileEntry) {
+CachedMainFileEntry = getFileEntryRefForID(MainFileID);
+assert(CachedMainFileEntry && "missing file entry for main file");
+  }
+  return CachedMainFileEntry->getUID() == SourceFile.getUID();
+}
+
 void SourceManager::initializeForReplay(const SourceManager ) {
   assert(MainFileID.isInvalid() && "expected uninitialized SourceManager");
 
Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -35,6 +35,7 @@
 #define LLVM_CLANG_BASIC_SOURCEMANAGER_H
 
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitVector.h"
@@ -59,9 +60,6 @@
 
 class ASTReader;
 class ASTWriter;
-class FileManager;
-class FileEntry;
-class FileEntryRef;
 class LineTableInfo;
 class SourceManager;
 
@@ -706,6 +704,9 @@
   /// The file ID for the main source file of the translation unit.
   FileID MainFileID;
 
+  /// The file entry for the main source file.
+  Optional CachedMainFileEntry;
+
   /// The file ID for the precompiled preamble there is one.
   FileID PreambleFileID;
 
@@ -813,6 +814,9 @@
 MainFileID = FID;
   }
 
+  /// Returns true when the given FileEntry corresponds to the main file.
+  bool isMainFile(FileEntryRef SourceFile);
+
   /// Set the file ID for the precompiled preamble.
   void setPreambleFileID(FileID Preamble) {
 assert(PreambleFileID.isInvalid() && "PreambleFileID already set!");


Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -2054,8 +2054,7 @@
   // some directives (e.g. #endif of a header guard) will never be seen.
   // Since this will lead to confusing errors, avoid the inclusion.
   if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
-  SourceMgr.translateFile(>getFileEntry()) ==
-  SourceMgr.getMainFileID()) {
+  SourceMgr.isMainFile(*File)) {
 Diag(FilenameTok.getLocation(),
  diag::err_pp_including_mainfile_in_preamble);
 return {ImportAction::None};
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -373,6 +373,7 @@
 
 void SourceManager::clearIDTables() {
   MainFileID = FileID();
+  CachedMainFileEntry = None;
   LocalSLocEntryTable.clear();
   LoadedSLocEntryTable.clear();
   SLocEntryLoaded.clear();
@@ -389,6 +390,15 @@