[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Serialization/ModuleManager.cpp:183
   // Get a buffer of the file and close the file descriptor when done.
-  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
 }

vsapsai wrote:
> dblaikie wrote:
> > vsapsai wrote:
> > > dexonsmith wrote:
> > > > rsmith wrote:
> > > > > dexonsmith wrote:
> > > > > > vsapsai wrote:
> > > > > > > Made this change because if we don't have a valid module but 
> > > > > > > opened a corresponding .pcm file earlier, there is a high chance 
> > > > > > > that .pcm file was rebuilt.
> > > > > > Please add a comment in the code explaining that.
> > > > > This change is proving really bad for us. This prevents using `mmap` 
> > > > > for loading module files, and instead forces the entire file to be 
> > > > > loaded every time. Please revert.
> > > > Can we limit the revert to explicit vs. implicit module builds?  The 
> > > > scenario Volodymyr was defending against is implicit-only.
> > > Richard, can you please tell what is your modules configuration and how 
> > > you are invoking clang? For implicit builds loading a module instead of 
> > > `mmap` is still better than building a module. But for explicit modules I 
> > > see there is no need to use `isVolatile` as modules aren't changing all 
> > > the time.
> > Richard/Google use explicit modules, if that's the particular parameter 
> > you're asking about.
> > 
> > So, yes, for Google's needs a solution that allows mmap to continue to be 
> > used for explicit modules would suffice, I believe.
> > 
> > (not to in any way derail that from being addressed - I thought implicit 
> > modules weren't race-y - they're hashed, etc, so they shouldn't collide/be 
> > overwritten? Seems like a loss in performance to have to copy the whole 
> > thing into memory rather than using mmap, if that could be avoided by more 
> > precise hashing/collision avoidance?)
> I need a specific command-line invocation to test with. If tests in 
> `clang/test/Modules/explicit*` are relevant to Google's setup, I can use 
> those as an example. It's just they were touched last time in 2016.
> 
> To discuss racy-ness, that is kinda inherent problem for incremental builds. 
> If a module becomes invalid, all of its users will try to get an up-to-date 
> version of it. And they can try to build that module themselves or check and 
> load already rebuilt module. Unfortunately, I don't see how different hashing 
> can help with it, after all multiple compiler invocations do want the same 
> .pcm file.
Preventing the use of `mmap` seems unacceptable in general -- for either 
implicit or explicit modules. Anything that forces compile time to be linear in 
the total size of transitive modules used is not reasonable. To really see the 
problems here you'll need a real world testcase with thousands to tens of 
thousands of modules, which I can't reasonably give you.

There should be ways we can avoid problems with racyness. Perhaps we could 
switch to a content-addressed module file store?
In any case, this patch has introduced a regression, so we should revert this 
change while we work out what to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[Volodymyr Sapsai via Phabricator via cfe-commits]

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



Comment at: clang/lib/Serialization/ModuleManager.cpp:183
   // Get a buffer of the file and close the file descriptor when done.
-  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
 }

dblaikie wrote:
> vsapsai wrote:
> > dexonsmith wrote:
> > > rsmith wrote:
> > > > dexonsmith wrote:
> > > > > vsapsai wrote:
> > > > > > Made this change because if we don't have a valid module but opened 
> > > > > > a corresponding .pcm file earlier, there is a high chance that .pcm 
> > > > > > file was rebuilt.
> > > > > Please add a comment in the code explaining that.
> > > > This change is proving really bad for us. This prevents using `mmap` 
> > > > for loading module files, and instead forces the entire file to be 
> > > > loaded every time. Please revert.
> > > Can we limit the revert to explicit vs. implicit module builds?  The 
> > > scenario Volodymyr was defending against is implicit-only.
> > Richard, can you please tell what is your modules configuration and how you 
> > are invoking clang? For implicit builds loading a module instead of `mmap` 
> > is still better than building a module. But for explicit modules I see 
> > there is no need to use `isVolatile` as modules aren't changing all the 
> > time.
> Richard/Google use explicit modules, if that's the particular parameter 
> you're asking about.
> 
> So, yes, for Google's needs a solution that allows mmap to continue to be 
> used for explicit modules would suffice, I believe.
> 
> (not to in any way derail that from being addressed - I thought implicit 
> modules weren't race-y - they're hashed, etc, so they shouldn't collide/be 
> overwritten? Seems like a loss in performance to have to copy the whole thing 
> into memory rather than using mmap, if that could be avoided by more precise 
> hashing/collision avoidance?)
I need a specific command-line invocation to test with. If tests in 
`clang/test/Modules/explicit*` are relevant to Google's setup, I can use those 
as an example. It's just they were touched last time in 2016.

To discuss racy-ness, that is kinda inherent problem for incremental builds. If 
a module becomes invalid, all of its users will try to get an up-to-date 
version of it. And they can try to build that module themselves or check and 
load already rebuilt module. Unfortunately, I don't see how different hashing 
can help with it, after all multiple compiler invocations do want the same .pcm 
file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Serialization/ModuleManager.cpp:183
   // Get a buffer of the file and close the file descriptor when done.
-  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
 }

vsapsai wrote:
> dexonsmith wrote:
> > rsmith wrote:
> > > dexonsmith wrote:
> > > > vsapsai wrote:
> > > > > Made this change because if we don't have a valid module but opened a 
> > > > > corresponding .pcm file earlier, there is a high chance that .pcm 
> > > > > file was rebuilt.
> > > > Please add a comment in the code explaining that.
> > > This change is proving really bad for us. This prevents using `mmap` for 
> > > loading module files, and instead forces the entire file to be loaded 
> > > every time. Please revert.
> > Can we limit the revert to explicit vs. implicit module builds?  The 
> > scenario Volodymyr was defending against is implicit-only.
> Richard, can you please tell what is your modules configuration and how you 
> are invoking clang? For implicit builds loading a module instead of `mmap` is 
> still better than building a module. But for explicit modules I see there is 
> no need to use `isVolatile` as modules aren't changing all the time.
Richard/Google use explicit modules, if that's the particular parameter you're 
asking about.

So, yes, for Google's needs a solution that allows mmap to continue to be used 
for explicit modules would suffice, I believe.

(not to in any way derail that from being addressed - I thought implicit 
modules weren't race-y - they're hashed, etc, so they shouldn't collide/be 
overwritten? Seems like a loss in performance to have to copy the whole thing 
into memory rather than using mmap, if that could be avoided by more precise 
hashing/collision avoidance?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[Volodymyr Sapsai via Phabricator via cfe-commits]

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



Comment at: clang/lib/Serialization/ModuleManager.cpp:183
   // Get a buffer of the file and close the file descriptor when done.
-  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
 }

dexonsmith wrote:
> rsmith wrote:
> > dexonsmith wrote:
> > > vsapsai wrote:
> > > > Made this change because if we don't have a valid module but opened a 
> > > > corresponding .pcm file earlier, there is a high chance that .pcm file 
> > > > was rebuilt.
> > > Please add a comment in the code explaining that.
> > This change is proving really bad for us. This prevents using `mmap` for 
> > loading module files, and instead forces the entire file to be loaded every 
> > time. Please revert.
> Can we limit the revert to explicit vs. implicit module builds?  The scenario 
> Volodymyr was defending against is implicit-only.
Richard, can you please tell what is your modules configuration and how you are 
invoking clang? For implicit builds loading a module instead of `mmap` is still 
better than building a module. But for explicit modules I see there is no need 
to use `isVolatile` as modules aren't changing all the time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

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

dexonsmith added inline comments.



Comment at: clang/lib/Serialization/ModuleManager.cpp:183
   // Get a buffer of the file and close the file descriptor when done.
-  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
 }

rsmith wrote:
> dexonsmith wrote:
> > vsapsai wrote:
> > > Made this change because if we don't have a valid module but opened a 
> > > corresponding .pcm file earlier, there is a high chance that .pcm file 
> > > was rebuilt.
> > Please add a comment in the code explaining that.
> This change is proving really bad for us. This prevents using `mmap` for 
> loading module files, and instead forces the entire file to be loaded every 
> time. Please revert.
Can we limit the revert to explicit vs. implicit module builds?  The scenario 
Volodymyr was defending against is implicit-only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Serialization/ModuleManager.cpp:183
   // Get a buffer of the file and close the file descriptor when done.
-  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
 }

dexonsmith wrote:
> vsapsai wrote:
> > Made this change because if we don't have a valid module but opened a 
> > corresponding .pcm file earlier, there is a high chance that .pcm file was 
> > rebuilt.
> Please add a comment in the code explaining that.
This change is proving really bad for us. This prevents using `mmap` for 
loading module files, and instead forces the entire file to be loaded every 
time. Please revert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG83f4c3af021c: [modules] Do not cache invalid state for 
modules that we attempted to load. (authored by vsapsai).

Changed prior to commit:
  https://reviews.llvm.org/D72860?vs=238562=238668#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860

Files:
  clang/include/clang/Serialization/InMemoryModuleCache.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/InMemoryModuleCache.cpp
  clang/lib/Serialization/ModuleManager.cpp
  clang/test/Modules/Inputs/implicit-invalidate-chain/A.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/B.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/C.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap
  clang/test/Modules/implicit-invalidate-chain.c
  clang/unittests/Frontend/FrontendActionTest.cpp
  clang/unittests/Serialization/InMemoryModuleCacheTest.cpp

Index: clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
===
--- clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
+++ clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
@@ -24,12 +24,11 @@
 
 TEST(InMemoryModuleCacheTest, initialState) {
   InMemoryModuleCache Cache;
-  EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
+  EXPECT_EQ(nullptr, Cache.lookupPCM("B"));
   EXPECT_FALSE(Cache.isPCMFinal("B"));
-  EXPECT_FALSE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-  EXPECT_DEATH(Cache.tryToDropPCM("B"), "PCM to remove is unknown");
+  EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown");
   EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown");
 #endif
 }
@@ -40,37 +39,33 @@
 
   InMemoryModuleCache Cache;
   EXPECT_EQ(RawB, ("B", std::move(B)));
-  EXPECT_EQ(InMemoryModuleCache::Tentative, Cache.getPCMState("B"));
   EXPECT_EQ(RawB, Cache.lookupPCM("B"));
   EXPECT_FALSE(Cache.isPCMFinal("B"));
-  EXPECT_FALSE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
   EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
-  EXPECT_DEATH(Cache.addBuiltPCM("B", getBuffer(2)),
-   "Trying to override tentative PCM");
+  EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)),
+   "Already has a non-final PCM");
 #endif
 }
 
-TEST(InMemoryModuleCacheTest, addBuiltPCM) {
+TEST(InMemoryModuleCacheTest, addFinalPCM) {
   auto B = getBuffer(1);
   auto *RawB = B.get();
 
   InMemoryModuleCache Cache;
-  EXPECT_EQ(RawB, ("B", std::move(B)));
-  EXPECT_EQ(InMemoryModuleCache::Final, Cache.getPCMState("B"));
+  EXPECT_EQ(RawB, ("B", std::move(B)));
   EXPECT_EQ(RawB, Cache.lookupPCM("B"));
   EXPECT_TRUE(Cache.isPCMFinal("B"));
-  EXPECT_FALSE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
   EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
-  EXPECT_DEATH(Cache.addBuiltPCM("B", getBuffer(2)),
+  EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)),
"Trying to override finalized PCM");
 #endif
 }
 
-TEST(InMemoryModuleCacheTest, tryToDropPCM) {
+TEST(InMemoryModuleCacheTest, tryToRemovePCM) {
   auto B1 = getBuffer(1);
   auto B2 = getBuffer(2);
   auto *RawB1 = B1.get();
@@ -78,27 +73,22 @@
   ASSERT_NE(RawB1, RawB2);
 
   InMemoryModuleCache Cache;
-  EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
   EXPECT_EQ(RawB1, ("B", std::move(B1)));
-  EXPECT_FALSE(Cache.tryToDropPCM("B"));
+  EXPECT_FALSE(Cache.tryToRemovePCM("B"));
   EXPECT_EQ(nullptr, Cache.lookupPCM("B"));
-  EXPECT_EQ(InMemoryModuleCache::ToBuild, Cache.getPCMState("B"));
   EXPECT_FALSE(Cache.isPCMFinal("B"));
-  EXPECT_TRUE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-  EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
-  EXPECT_DEATH(Cache.tryToDropPCM("B"),
-   "PCM to remove is scheduled to be built");
-  EXPECT_DEATH(Cache.finalizePCM("B"), "Trying to finalize a dropped PCM");
+  EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown");
+  EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown");
 #endif
 
   // Add a new one.
-  EXPECT_EQ(RawB2, ("B", std::move(B2)));
+  EXPECT_EQ(RawB2, ("B", std::move(B2)));
   EXPECT_TRUE(Cache.isPCMFinal("B"));
 
   // Can try to drop again, but this should error and do nothing.
-  EXPECT_TRUE(Cache.tryToDropPCM("B"));
+  EXPECT_TRUE(Cache.tryToRemovePCM("B"));
   EXPECT_EQ(RawB2, Cache.lookupPCM("B"));
 }
 
@@ -107,12 +97,10 @@
   auto *RawB = B.get();
 
   InMemoryModuleCache Cache;
-  EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
   EXPECT_EQ(RawB, ("B", std::move(B)));
 
   // Call finalize.
   Cache.finalizePCM("B");
-  EXPECT_EQ(InMemoryModuleCache::Final, 

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

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

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

LGTM.




Comment at: clang/lib/Serialization/ModuleManager.cpp:183
   // Get a buffer of the file and close the file descriptor when done.
-  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
 }

vsapsai wrote:
> Made this change because if we don't have a valid module but opened a 
> corresponding .pcm file earlier, there is a high chance that .pcm file was 
> rebuilt.
Please add a comment in the code explaining that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61863 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked an inline comment as done.
vsapsai added a comment.

Anecdotal build time measurements before and after the change. First row is a 
clean build, subsequent rows are incremental builds.

| Revision | Before change | After change | Change (after - before) | Relative 
change |
|  | - |  | --- | 
--- |
| 5da385fb56c 
| 
27:40 | 28:20| +40s| +2.41%  |
| d4e006e8446 
| 
30:50 | 28:29| -141s   | -7.62%  |
| 77d049d0c65 
| 
0:28  | 0:28 | 0s  | 0%  |
| 1b9ef3bbb59 
| 
0:10  | 0:11 | +1s | +10%|
| ab411801b82 
| 
11:34 | 11:15| -19s| -2.74%  |
|

Cannot claim huge build time improvements but seems like there are no real 
regressions.




Comment at: clang/lib/Serialization/ModuleManager.cpp:183
   // Get a buffer of the file and close the file descriptor when done.
-  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+  Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true);
 }

Made this change because if we don't have a valid module but opened a 
corresponding .pcm file earlier, there is a high chance that .pcm file was 
rebuilt.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72860



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

[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: bruno, dexonsmith.
Herald added subscribers: cfe-commits, ributzka, jkorous.
Herald added a project: clang.

Partially reverts 0a2be46cfdb698fefcc860a56b47dde0884d5335 as it turned
out to cause redundant module rebuilds in multi-process incremental builds.
When a module was getting out of date, all compilation processes started at the
same time were marking it as `ToBuild`. So each process was building the same
module instead of checking if it was built by someone else and using that
result. In addition to the work duplication, contention on the same .pcm file
wasn't making builds faster.

Note that for a single-process build this change would cause redundant module
reads and validations. But reading a module is faster than building it and
multi-process builds are more common than single-process. So I'm willing to
make such a trade-off.

rdar://problem/54395127


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72860

Files:
  clang/include/clang/Serialization/InMemoryModuleCache.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/InMemoryModuleCache.cpp
  clang/lib/Serialization/ModuleManager.cpp
  clang/test/Modules/Inputs/implicit-invalidate-chain/A.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/B.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/C.h
  clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap
  clang/test/Modules/implicit-invalidate-chain.c
  clang/unittests/Frontend/FrontendActionTest.cpp
  clang/unittests/Serialization/InMemoryModuleCacheTest.cpp

Index: clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
===
--- clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
+++ clang/unittests/Serialization/InMemoryModuleCacheTest.cpp
@@ -24,12 +24,11 @@
 
 TEST(InMemoryModuleCacheTest, initialState) {
   InMemoryModuleCache Cache;
-  EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
+  EXPECT_EQ(nullptr, Cache.lookupPCM("B"));
   EXPECT_FALSE(Cache.isPCMFinal("B"));
-  EXPECT_FALSE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-  EXPECT_DEATH(Cache.tryToDropPCM("B"), "PCM to remove is unknown");
+  EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown");
   EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown");
 #endif
 }
@@ -40,37 +39,33 @@
 
   InMemoryModuleCache Cache;
   EXPECT_EQ(RawB, ("B", std::move(B)));
-  EXPECT_EQ(InMemoryModuleCache::Tentative, Cache.getPCMState("B"));
   EXPECT_EQ(RawB, Cache.lookupPCM("B"));
   EXPECT_FALSE(Cache.isPCMFinal("B"));
-  EXPECT_FALSE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
   EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
-  EXPECT_DEATH(Cache.addBuiltPCM("B", getBuffer(2)),
-   "Trying to override tentative PCM");
+  EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)),
+   "Already has a non-final PCM");
 #endif
 }
 
-TEST(InMemoryModuleCacheTest, addBuiltPCM) {
+TEST(InMemoryModuleCacheTest, addFinalPCM) {
   auto B = getBuffer(1);
   auto *RawB = B.get();
 
   InMemoryModuleCache Cache;
-  EXPECT_EQ(RawB, ("B", std::move(B)));
-  EXPECT_EQ(InMemoryModuleCache::Final, Cache.getPCMState("B"));
+  EXPECT_EQ(RawB, ("B", std::move(B)));
   EXPECT_EQ(RawB, Cache.lookupPCM("B"));
   EXPECT_TRUE(Cache.isPCMFinal("B"));
-  EXPECT_FALSE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
   EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
-  EXPECT_DEATH(Cache.addBuiltPCM("B", getBuffer(2)),
+  EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)),
"Trying to override finalized PCM");
 #endif
 }
 
-TEST(InMemoryModuleCacheTest, tryToDropPCM) {
+TEST(InMemoryModuleCacheTest, tryToRemovePCM) {
   auto B1 = getBuffer(1);
   auto B2 = getBuffer(2);
   auto *RawB1 = B1.get();
@@ -78,27 +73,22 @@
   ASSERT_NE(RawB1, RawB2);
 
   InMemoryModuleCache Cache;
-  EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
   EXPECT_EQ(RawB1, ("B", std::move(B1)));
-  EXPECT_FALSE(Cache.tryToDropPCM("B"));
+  EXPECT_FALSE(Cache.tryToRemovePCM("B"));
   EXPECT_EQ(nullptr, Cache.lookupPCM("B"));
-  EXPECT_EQ(InMemoryModuleCache::ToBuild, Cache.getPCMState("B"));
   EXPECT_FALSE(Cache.isPCMFinal("B"));
-  EXPECT_TRUE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-  EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM");
-  EXPECT_DEATH(Cache.tryToDropPCM("B"),
-   "PCM to remove is scheduled to be built");
-  EXPECT_DEATH(Cache.finalizePCM("B"), "Trying to finalize a dropped PCM");
+  EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown");
+  EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown");
 #endif
 
   // Add a new one.
-  EXPECT_EQ(RawB2, ("B",