[C++20][Modules] Introduce an implementation module.

[iains]

Summary

We need to be able to distinguish individual TUs from the same module in cases
where TU-local entities either need to be hidden (or, for some cases of ADL in
template instantiation, need to be detected as exposures).

This creates a module type for the implementation which implicitly imports its
primary module interface per C++20:
[module.unit/8] 'A module-declaration that contains neither an export-keyword
nor a module-partition implicitly imports the primary module interface unit of
the module as if by a module-import-declaration.

Implementation modules are never serialized (-emit-module-interface for an
implementation unit is diagnosed and rejected).

[iains]

we need to distinguish implementation and interface in code-gen.

[tahonermann]

Implementation modules are never serialized (-emit-module-interface for an implementation unit is diagnosed and rejected).

Never? Or just not via -emit-module-interface? I would expect to be able to serialize via -emit-ast.

[iains]

In D126959#3556074, @tahonermann wrote:

Implementation modules are never serialized (-emit-module-interface for an implementation unit is diagnosed and rejected).

Never? Or just not via -emit-module-interface? I would expect to be able to serialize via -emit-ast.

My current intent is that this is a implementation detail (a mechanism for maintaining the module purview and type for Sema and CG).

As things stand, I do not think we are set up to handle (de-)serialization of two kinds of module with the same name, so I'd prefer to defer
possible extension to allow this for now (not against the principle, just trying avoid a rabbit hole that's tangential to the current purpose).

[ChuanqiXu]

clang/lib/Lex/ModuleMap.cpp

944

The implementation looks similar to createModuleForInterfaceUnit really. It looks better to refactor it to createModuleUnits (or createModuleForCXX20Modules) which containing an additional argument Module::ModuleKind. It would optimize the case for partitions too, which uses createModuleForInterfaceUnit now and it is a little bit odd.

clang/lib/Sema/SemaModule.cpp

354

I feel like the 3 comments are redundant.

392–396

We could move this logic to the place Import is created.

419–420

Is it necessary/helpful to return the import declaration? Although there is a FIXME, I think the current method looks a little bit confusing.

[ChuanqiXu]

In D126959#3556115, @iains wrote:

In D126959#3556074, @tahonermann wrote:

Implementation modules are never serialized (-emit-module-interface for an implementation unit is diagnosed and rejected).

Never? Or just not via -emit-module-interface? I would expect to be able to serialize via -emit-ast.

My current intent is that this is a implementation detail (a mechanism for maintaining the module purview and type for Sema and CG).

As things stand, I do not think we are set up to handle (de-)serialization of two kinds of module with the same name, so I'd prefer to defer
possible extension to allow this for now (not against the principle, just trying avoid a rabbit hole that's tangential to the current purpose).

I think Iain's "(de-)serialization" means "generating PCM files". And it makes no sense to block users to use emit-ast for implementation units. I don't think we have divergence here since I don't find codes which try to emit an error if we are attempting to generate a PCM file for an implementation unit.

---

I feel better if we could add some tests:

• As mentioned above, emit an error when we trying to generate a PCM file for an implementation unit.
• Currently, when we use implementation units, we need to specify -fmodule-file=/path/to/the/pcm/of/primary/interface in the command line. It looks like we could omit it after the patch (as long as the PCM file of primary interface lives in the path specified by -fprebuilt-module-path)

clang/lib/Frontend/FrontendActions.cpp

763

Since ModuleKindName() here is used as a helpful for printers, it looks odd to contain the should never be serialized part. I think we should check this when we try to generate PCM files for ModuleImplementationUnit.

[iains]

thanks for the reviews, I need to figure out why the implementation passes clang tests but gives fails for clangd.

[iains]

rebased and tidied (still changes planned).

[iains]

rebased, and reworked for changes in main.

[iains]

This was originally created (and @ChuanqiXu approved) for the work on module initialisers. I did not apply it then, since it was possible to determine the correct state for the initialisers without it.

However, now we are trying to handle some of the details of different lookup requirements between same-module and same-TU, I think it is a foundation for that. This also means that implementation units will get their own TU decl and have a distinct decl context (rather than the current case that we share the decl context of the interface).

The units are never serialised (as noted above) but we still need to allocate an extra bit for the enum (since we now have 9 values); this is not a significant load (since it adds only one bit to each module header).

[ChuanqiXu]

(I don't why I can't send emails in the original mail address. It told me that your email addressed is not recorded by MX, while I don't know what is MX. So I tried to reply your email here)

This FIXME was added 6 years ago for Modules TS. (https://github.com/llvm/llvm-project/commit/becb92dec85924969ac0c3b049e0a74def431453).
And I moved the test simply without modifying it actually.

And I guess it may refer to the problem we are discussing. The spec says the implementation unit will import the primary module interface implicitly.
But the current implementation doesn't import it actually but loading it. This is inconsistent with the wording and it is not consistent with the design intention in clang.
It resulted some problems we saw.

[ChuanqiXu]

I don't remember clearly if we have a test for the initializer in the implementation unit. Ignore this if we already have one.

clang/include/clang/Basic/Module.h

137

We may be able to save 1 bit for ModuleKind by adding two field bits to Module: IsImplementation and IsPartition. Then we can remove ModulePartitionInterface and ModulePartitionImplementation. Then let's rename ModuleInterfaceUnit to NamedModuleUnit.
So we can judge module interface unit, implementation unit, module partition interface and module partition implementation by NamedModuleUnit and the two bits.

clang/lib/Sema/SemaDecl.cpp

1672–1677

I feel slightly better to add a field in Sema:

Module *PrimaryModuleInterface = nullptr; // Only valid if we're parsing a module unit.

Then we can avoid the string compare here. Also we can add it to Sema::isUsableModule. Now Sema::isUsableModule works because it falls to the string comparison.

clang/lib/Sema/SemaModule.cpp

302

Then we can avoid to declare this if we have Sema::PrimaryModuleInterface

408–409

ModuleScopes can't be empty here.

clang/test/CXX/module/basic/basic.def.odr/p4.cppm

155–156

nit: I am a little bit tired to add FIXME in the tests... let's not try to add more...

[iains]

General comments (at least my opinion).

1. One intention of this patch is to make Implementation Module Units regular (i.e. they should behave the same as any other module unit). So I would tend to avoid changes that make this back into a "special case"

2. The lookup problems might be considered to be quite a serious bug, and so we should consider possible back porting and try to avoid making large changes in theses patches (once that problem is solved, then we can refactor, I think).

3. We do have tests for the initialiser in implementation units.

In D126959#4201152, @ChuanqiXu wrote:

And I guess it may refer to the problem we are discussing. The spec says the implementation unit will import the primary module interface implicitly.
But the current implementation doesn't import it actually but loading it. This is inconsistent with the wording and it is not consistent with the design intention in clang.
It resulted some problems we saw.

This patch implements the remaining pieces of that (including "as if by an import).

clang/include/clang/Basic/Module.h

137

Yes I agree we could do this, but let's keep refactoring as a follow-on job.

clang/lib/Frontend/FrontendActions.cpp

763

OK. I can remove that

We do already check and refuse to generate a PCM for a module implementation (if you put -emit-module-interface, and the file contains module Foo; it will be rejected).

clang/lib/Lex/ModuleMap.cpp

944

OK I did think about refactoring that, but decided to keep the changes simple for now. I am happy to look at refactoring.

clang/lib/Sema/SemaDecl.cpp

1672–1677

I also thought about doing this, but decided that we should try to keep the interface regular - so that all modules are treated the same.

If we find that the string comparison for primary module name is a "hot spot" then we should address it a different way - by caching an identifier or similar (since we need to compare it in other places too).

clang/lib/Sema/SemaModule.cpp

302

see comment above.

354

OK..

392–396

We cannot move all of it - since some of the information is being added to the module that we create after the import.

408–409

OK.

419–420

The FIXME predates our work on this, at the moment I do not know what the idea was for that - I am happy to remove it now - the current implementation does not need a module decl.

clang/test/CXX/module/basic/basic.def.odr/p4.cppm

155–156

I agree that FIXMEs in tests are likely to be forgotten, so that they are not so much use.

However, we have a situation in which the test needs to be incorrect in order to pass at the present time; we do not have a good mechanism for noting this - since if we add the expected-error, then the test will fail - so we'd have to XFAIL it - which would mean that we would lose test coverage for the cases that do work.

If you prefer, I can remove the FIXME.

[ChuanqiXu]

The lookup problems might be considered to be quite a serious bug, and so we should consider possible back porting and try to avoid making large changes in theses patches (once that problem is solved, then we can refactor, I think).

I agree it is important to fix the bug. But I don't think we should back port this one to 16. On the one hand, this is not a regression bug. On the other hand, the existing users (we already have a lot users now) didn't report such problems. So I prefer to target 17 for this. And I don't mind to do refactoring after landing the fixing patch. Since we're looking at the code actively.

clang/include/clang/Basic/Module.h

137

OK. I don't think this is a blocking issue too. Let's have one FIXME for it.

clang/lib/Sema/SemaDecl.cpp

1672–1677

It just smells bad. I did profiling for the performance for modules these days. And it looks like the string comparison can hard to be the hot spot. The bottleneck lives in other places. The string comparison may be in the long tail. So we might not be able to prove this one by giving numbers. But it indeed smells bad and we can solve it simply. So let's make it. I think it is important to improve our code quality.

clang/test/CXX/module/basic/basic.def.odr/p4.cppm

155–156

I don't feel bad to remain this one. Since it is a little bit special as you said.

[iains]

rebased, addressed review comments, and amended the description.

the patch has been repurposed to provide a mechanism to track implementation
TUs as separate entities which allows us to deal with handling Tu-local
entities better.

[iains]

clang/lib/Sema/SemaDecl.cpp

1672–1677

I have done this (for now) to make progress.

but, actually, in this case, I do not agree with the mechanism

.. instead of making this into a special case we should (as part of the following fixes to lookup) make efficient helpers in module / sema that allow us to see "same TU", "same named module" etc. That way we can improve the performance of any one of those if it becomes an issue.

[ChuanqiXu]

LGTM. Thanks.

clang/include/clang/Sema/Sema.h

2278 ↗

(On Diff #507535)

clang/lib/Sema/SemaDecl.cpp

1672–1677

Yeah, I tried to use hash algorithm to avoid the string comparison. But there is a concern that the hash algorithm is not safe. And I found the string comparison is not the bottle neck then. So I think it may not be too late to perform it actually if there is related bug report.

[bjope]

This seem to case problems when building with asan enabled (LLVM_USE_SANITIZER='Address'):

Failed Tests (24):
  Clang :: CXX/basic/basic.link/p2.cpp
  Clang :: CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp
  Clang :: CXX/basic/basic.scope/basic.scope.namespace/p2.cpp
  Clang :: CXX/module/basic/basic.def.odr/p4.cppm
  Clang :: CXX/module/basic/basic.def.odr/p6.cppm
  Clang :: CXX/module/basic/basic.link/module-declaration.cpp
  Clang :: CXX/module/basic/basic.link/p2.cppm
  Clang :: CXX/module/dcl.dcl/dcl.module/dcl.module.import/p1.cppm
  Clang :: CXX/module/dcl.dcl/dcl.module/dcl.module.interface/p1.cppm
  Clang :: CXX/module/dcl.dcl/dcl.module/p1.cpp
  Clang :: CXX/module/dcl.dcl/dcl.module/p5.cpp
  Clang :: CXX/module/module.interface/p1.cpp
  Clang :: CXX/module/module.interface/p2.cpp
  Clang :: CXX/module/module.unit/p8.cpp
  Clang :: CodeGenCXX/cxx20-module-impl-1a.cpp
  Clang :: CodeGenCXX/module-intializer.cpp
  Clang :: Modules/cxx20-10-1-ex1.cpp
  Clang :: Modules/cxx20-10-1-ex2.cpp
  Clang :: Modules/cxx20-10-2-ex5.cpp
  Clang :: Modules/cxx20-10-3-ex2.cpp
  Clang :: Modules/cxx20-impl-module-conditionally-load.cppm
  Clang :: Modules/cxx20-import-diagnostics-a.cpp
  Clang :: Modules/cxx20-partition-redeclarations.cpp
  Clang :: Modules/pr58532.cppm

Here is a typical log from Modules/cxx20-partition-redeclarations.cpp:

==80746==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2080 byte(s) in 1 object(s) allocated from:
    #0 0x61ea6d in operator new(unsigned long) /repo/old//compiler-rt/lib/asan/asan_new_delete.cpp:99:3
    #1 0x11a9ae7a in clang::ModuleMap::createModuleUnitWithKind(clang::SourceLocation, llvm::StringRef, clang::Module::ModuleKind) /repo/clang/lib/Lex/ModuleMap.cpp:894:7
    #2 0x11a9bc57 in clang::ModuleMap::createModuleForImplementationUnit(clang::SourceLocation, llvm::StringRef) /repo/clang/lib/Lex/ModuleMap.cpp:933:7
    #3 0xe84a76d in clang::Sema::ActOnModuleDecl(clang::SourceLocation, clang::SourceLocation, clang::Sema::ModuleDeclKind, llvm::ArrayRef<std::__1::pair<clang::IdentifierInfo*, clang::SourceLocation>>, llvm::ArrayRef<std::__1::pair<clang::IdentifierInfo*, clang::SourceLocation>>, clang::Sema::ModuleImportState&) /repo/clang/lib/Sema/SemaModule.cpp:350:17
    #4 0xd347fc5 in clang::Parser::ParseModuleDecl(clang::Sema::ModuleImportState&) /repo/clang/lib/Parse/Parser.cpp:2478:18
    #5 0xd3451b4 in clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /repo/clang/lib/Parse/Parser.cpp:658:14
    #6 0xd343014 in clang::Parser::ParseFirstTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, clang::Sema::ModuleImportState&) /repo/clang/lib/Parse/Parser.cpp:593:26
    #7 0xd32c624 in clang::ParseAST(clang::Sema&, bool, bool) /repo/clang/lib/Parse/ParseAST.cpp:161:25
    #8 0x9a0a494 in clang::FrontendAction::Execute() /repo/clang/lib/Frontend/FrontendAction.cpp:1058:8
    #9 0x98025a0 in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /repo/clang/lib/Frontend/CompilerInstance.cpp:1048:33
    #10 0x9ca0f3d in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /repo/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:264:25
    #11 0x63ad58 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /repo/clang/tools/driver/cc1_main.cpp:251:15
    #12 0x62d5cc in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) /repo/clang/tools/driver/driver.cpp:366:12
    #13 0x628173 in clang_main(int, char**, llvm::ToolContext const&) /repo/clang/tools/driver/driver.cpp:407:12
    #14 0x65de41 in main /repo/tools/clang/tools/driver/clang-driver.cpp:15:10
    #15 0x7fd7c0406554 in __libc_start_main (/lib64/libc.so.6+0x22554)

SUMMARY: AddressSanitizer: 2080 byte(s) leaked in 1 allocation(s).

It is for example seen in this buildbot (sanitizer-x86_64-linux-bootstrap-asan): https://lab.llvm.org/buildbot/#/builders/168

[iains]

In D126959#4222637, @bjope wrote:

This seem to case problems when building with asan enabled (LLVM_USE_SANITIZER='Address'):

investigating... do you need the patch reverted?

[bjope]

In D126959#4222854, @iains wrote:

In D126959#4222637, @bjope wrote:

This seem to case problems when building with asan enabled (LLVM_USE_SANITIZER='Address'):

investigating... do you need the patch reverted?

I've reverted it downstream right now, so personally I don't have a problem with this right now. Still, unless it will go quickly to find a fix it might help others people if reverting (it was not obvious to me which patch that caused the failures, and others might still be bisecting trying to find which commit that caused the trouble).

[hctim]

Hi, yep, I've reverted upstream. If you could please also integrate https://reviews.llvm.org/rG8c7c1f11ffaacf762e612c65440fd2cbb58ee426 in the relanding, that would be great.

[iains]

I had a hunch that the issue was the non-ownership of the implementation module.

[iains]

rebased, reworked to have the module owned.

The implementation module needs to be owned by the mpodul map so that
it is released when done. Reworked the comments and assert to check the
main file presence.

[iains]

It took me a while to get my local macOS based devt. environment to reproduce the problem - but it was as expected; the implementation module was unowned and nothing was deleting it.

unless CI throws up anything untoward, I plan to re-land this tomorrow (since there is dependent work in my queue and this blocks or at least impedes progress there).

[iains]

rebased, retested.

[ChuanqiXu]

clang/lib/Lex/ModuleMap.cpp

928

nit: It should more consistent to use <ImplementationUnit>.

[iains]

clang/lib/Lex/ModuleMap.cpp

928

(I do not mind making this change later, if you like - but I did not want to repeat the test cycle today).

In this case I think that <> would not mean the same thing as it does in <global> and <private> modules fragments, These are never entered into the module map - but are always owned by a parent module.

In the case of the ImplementationUnit - the *name* of the module is unchanged (i.e. it would still be called M when the module line is module M;.

The .ImplementationUnit is not the name of the module - but rather it is a key into the module map (which needs to be different from the name of the interface), Since there can only be one Implementation Unit in a given session, it is safe to use a fixed key.

However, I do not mind changing the key to <ImplementationUnit> if you think that it would be more clear.

[ChuanqiXu]

clang/lib/Lex/ModuleMap.cpp

928

Thanks, I think it doesn't matter too.

clang/include/clang/Basic/Module.h

public:
   /// The location of the module definition.
   SourceLocation DefinitionLoc;
 
   enum ModuleKind {
     /// This is a module that was defined by a module map and built out
     /// of header files.
     ModuleMapModule,
 
+    /// This is a C++ 20 header unit.
+    ModuleHeaderUnit,
+
     /// This is a C++20 module interface unit.
     ModuleInterfaceUnit,
 
-    /// This is a C++ 20 header unit.
-    ModuleHeaderUnit,
+    /// This is a C++20 module implementation unit.
+    ModuleImplementationUnit,
 
     /// This is a C++ 20 module partition interface.
     ModulePartitionInterface,
 
     /// This is a C++ 20 module partition implementation.
     ModulePartitionImplementation,
 
     /// This is the explicit Global Module Fragment of a modular TU.
     /// As per C++ [module.global.frag].
     ExplicitGlobalModuleFragment,
 
     /// This is the private module fragment within some C++ module.
     PrivateModuleFragment,
 
     /// This is an implicit fragment of the global module which contains
     /// only language linkage declarations (made in the purview of the
     /// named module).
     ImplicitGlobalModuleFragment,
   };
 
   /// The kind of this module.
   ModuleKind Kind = ModuleMapModule;
 
   /// The parent of this module. This will be NULL for the top-level
   /// module.
   Module *Parent;
 
public:
 
   /// The module through which entities defined in this module will
   /// eventually be exposed, for use in "private" modules.
   std::string ExportAsModule;
 
   /// Does this Module scope describe part of the purview of a standard named
   /// C++ module?
   bool isModulePurview() const {
-    return Kind == ModuleInterfaceUnit || Kind == ModulePartitionInterface ||
-           Kind == ModulePartitionImplementation ||
-           Kind == PrivateModuleFragment;
+    switch (Kind) {
+    case ModuleInterfaceUnit:
+    case ModuleImplementationUnit:
+    case ModulePartitionInterface:
+    case ModulePartitionImplementation:
+    case PrivateModuleFragment:
+      return true;
+    default:
+      return false;
+    }
   }
 
   /// Does this Module scope describe a fragment of the global module within
   /// some C++ module.
   bool isGlobalModule() const {
     return isExplicitGlobalModule() || isImplicitGlobalModule();
   }
   bool isExplicitGlobalModule() const {
public:
   }
 
   /// Is this a module partition.
   bool isModulePartition() const {
     return Kind == ModulePartitionInterface ||
            Kind == ModulePartitionImplementation;
   }
 
+  /// Is this a module implementation.
+  bool isModuleImplementation() const {
+    return Kind == ModuleImplementationUnit;
+  }
+
   /// Is this module a header unit.
   bool isHeaderUnit() const { return Kind == ModuleHeaderUnit; }
   // Is this a C++20 module interface or a partition.
   bool isInterfaceOrPartition() const {
     return Kind == ModuleInterfaceUnit || isModulePartition();
   }
 
   bool isModuleInterfaceUnit() const {

clang/include/clang/Lex/ModuleMap.h

public:
   /// The module must not already exist, and will be configured for the current
   /// compilation.
   ///
   /// Note that this also sets the current module to the newly-created module.
   ///
   /// \returns The newly-created module.
   Module *createModuleForInterfaceUnit(SourceLocation Loc, StringRef Name);
 
+  /// Create a new module for a C++ module implementation unit.
+  /// The interface module for this implementation (implicitly imported) must
+  /// exist and be loaded and present in the modules map.
+  ///
+  /// \returns The newly-created module.
+  Module *createModuleForImplementationUnit(SourceLocation Loc, StringRef Name);
+
   /// Create a C++20 header unit.
   Module *createHeaderUnit(SourceLocation Loc, StringRef Name,
                            Module::Header H);
 
   /// Infer the contents of a framework module map from the given
   /// framework directory.
   Module *inferFrameworkModule(const DirectoryEntry *FrameworkDir,
                                bool IsSystem, Module *Parent);

clang/lib/AST/Decl.cpp

if (!M)
     return nullptr;
 
   switch (M->Kind) {
   case Module::ModuleMapModule:
     // Module map modules have no special linkage semantics.
     return nullptr;
 
   case Module::ModuleInterfaceUnit:
+  case Module::ModuleImplementationUnit:
   case Module::ModulePartitionInterface:
   case Module::ModulePartitionImplementation:
     return M;
 
   case Module::ModuleHeaderUnit:
   case Module::ExplicitGlobalModuleFragment:
   case Module::ImplicitGlobalModuleFragment: {
     // External linkage declarations in the global module have no owning module

clang/lib/CodeGen/CGDeclCXX.cpp

if (getCXXABI().useSinitAndSterm() && ModuleInits.empty() &&
     return;
 
   for (auto *F : CXXGlobalInits)
     ModuleInits.push_back(F);
   CXXGlobalInits.clear();
 
   // Include the filename in the symbol name. Including "sub_" matches gcc
   // and makes sure these symbols appear lexicographically behind the symbols
-  // with priority emitted above.
+  // with priority emitted above.  Module implementation units behave the same
+  // way as a non-modular TU with imports.
   llvm::Function *Fn;
-  if (CXX20ModuleInits && getContext().getNamedModuleForCodeGen()) {
+  if (CXX20ModuleInits && getContext().getNamedModuleForCodeGen() &&
+      !getContext().getNamedModuleForCodeGen()->isModuleImplementation()) {
     SmallString<256> InitFnName;
     llvm::raw_svector_ostream Out(InitFnName);
     cast<ItaniumMangleContext>(getCXXABI().getMangleContext())
         .mangleModuleInitializer(getContext().getNamedModuleForCodeGen(), Out);
     Fn = CreateGlobalInitOrCleanUpFunction(
         FTy, llvm::Twine(InitFnName), FI, SourceLocation(), false,
         llvm::GlobalVariable::ExternalLinkage);
   } else

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::Release() {
 
   if (Context.getLangOpts().IncrementalExtensions &&
       GlobalTopLevelStmtBlockInFlight.first) {
     const TopLevelStmtDecl *TLSD = GlobalTopLevelStmtBlockInFlight.second;
     GlobalTopLevelStmtBlockInFlight.first->FinishFunction(TLSD->getEndLoc());
     GlobalTopLevelStmtBlockInFlight = {nullptr, nullptr};
   }
 
+  // Module implementations are initialized the same way as a regular TU that
+  // imports one or more modules.
   if (CXX20ModuleInits && Primary && Primary->isInterfaceOrPartition())
     EmitCXXModuleInitFunc(Primary);
   else
     EmitCXXGlobalInitFunc();
   EmitCXXGlobalCleanUpFunc();
   registerGlobalDtorsWithAtExit();
   EmitCXXThreadLocalInitFunc();
   if (ObjCRuntime)

clang/lib/Frontend/FrontendActions.cpp

 }
 
 static StringRef ModuleKindName(Module::ModuleKind MK) {
   switch (MK) {
   case Module::ModuleMapModule:
     return "Module Map Module";
   case Module::ModuleInterfaceUnit:
     return "Interface Unit";
+  case Module::ModuleImplementationUnit:
+    return "Implementation Unit (should never be serialized)";
   case Module::ModulePartitionInterface:
     return "Partition Interface";
   case Module::ModulePartitionImplementation:
     return "Partition Implementation";
   case Module::ModuleHeaderUnit:
     return "Header Unit";
   case Module::ExplicitGlobalModuleFragment:
     return "Global Module Fragment";

clang/lib/Lex/ModuleMap.cpp

Module *ModuleMap::createModuleForInterfaceUnit(SourceLocation Loc,
   // declarations and macros are properly visibility-restricted to it.
   auto *MainFile = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
   assert(MainFile && "no input file for module interface");
   Headers[MainFile].push_back(KnownHeader(Result, PrivateHeader));
 
   return Result;
 }
 
+Module *ModuleMap::createModuleForImplementationUnit(SourceLocation Loc,
+                                                     StringRef Name) {
+  assert(LangOpts.CurrentModule == Name && "module name mismatch");
+  // The interface for this implementation must exist and be loaded.
+  assert(Modules[Name] && Modules[Name]->Kind == Module::ModuleInterfaceUnit &&
+         "creating implementation module without an interface");
+
+  auto *Result = new Module(Name, Loc, nullptr, /*IsFramework*/ false,
+                            /*IsExplicit*/ false, NumCreatedModules++);
+  Result->Kind = Module::ModuleImplementationUnit;
+  SourceModule = Result;
+
+  // Reparent the current global module fragment as a submodule of this module.
+  for (auto &Submodule : PendingSubmodules) {
+    Submodule->setParent(Result);
+    Submodule.release(); // now owned by parent
+  }
+  PendingSubmodules.clear();
+
+  // Mark the main source file as being within the newly-created module so that
+  // declarations and macros are properly visibility-restricted to it.
+  auto *MainFile = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
+  assert(MainFile && "no input file for module implementation");
+
+  return Result;
+}
+
 Module *ModuleMap::createHeaderUnit(SourceLocation Loc, StringRef Name,
                                     Module::Header H) {
   assert(LangOpts.CurrentModule == Name && "module name mismatch");
   assert(!Modules[Name] && "redefining existing module");
 
   auto *Result = new Module(Name, Loc, nullptr, /*IsFramework*/ false,
                             /*IsExplicit*/ false, NumCreatedModules++);
   Result->Kind = Module::ModuleHeaderUnit;

clang/lib/Sema/SemaDecl.cpp

bool Sema::CheckRedeclarationModuleOwnership(NamedDecl *New, NamedDecl *Old) {
   // Partitions are part of the module, but a partition could import another
   // module, so verify that the PMIs agree.
   if (NewM && OldM &&
       (NewM->isModulePartition() || OldM->isModulePartition()) &&
       NewM->getPrimaryModuleInterfaceName() ==
           OldM->getPrimaryModuleInterfaceName())
     return false;
 
+  // A module implementation unit has visibility of the decls in its implicitly
+  // imported interface.
+  if (getLangOpts().CPlusPlusModules && NewM && OldM &&
+      NewM->Kind == Module::ModuleImplementationUnit &&
+      OldM->Kind == Module::ModuleInterfaceUnit && NewM->Name == OldM->Name)
+    return false;
+
   bool NewIsModuleInterface = NewM && NewM->isModulePurview();
   bool OldIsModuleInterface = OldM && OldM->isModulePurview();
   if (NewIsModuleInterface || OldIsModuleInterface) {
     // C++ Modules TS [basic.def.odr] 6.2/6.7 [sic]:
     //   if a declaration of D [...] appears in the purview of a module, all
     //   other such declarations shall appear in the purview of the same module
     Diag(New->getLocation(), diag::err_mismatched_owning_module)
       << New

clang/lib/Sema/SemaModule.cpp

Diag(Path.front().second, diag::err_current_module_name_mismatch)
                                                 ? Partition.back().second
                                                 : Path.back().second)
         << getLangOpts().CurrentModule;
     return nullptr;
   }
   const_cast<LangOptions&>(getLangOpts()).CurrentModule = ModuleName;
 
   auto &Map = PP.getHeaderSearchInfo().getModuleMap();
-  Module *Mod;
-
+  Module *Mod;                 // The module we are creating.
+  Module *Interface = nullptr; // The interface for an implementation.
   switch (MDK) {
   case ModuleDeclKind::Interface:
   case ModuleDeclKind::PartitionInterface: {
     // We can't have parsed or imported a definition of this module or parsed a
     // module map defining it already.
     if (auto *M = Map.findModule(ModuleName)) {
       Diag(Path[0].second, diag::err_module_redefinition) << ModuleName;
       if (M->DefinitionLoc.isValid())
case ModuleDeclKind::Implementation: {
     std::pair<IdentifierInfo *, SourceLocation> ModuleNameLoc(
         PP.getIdentifierInfo(ModuleName), Path[0].second);
 
     // The module loader will assume we're trying to import the module that
     // we're building if `LangOpts.CurrentModule` equals to 'ModuleName'.
     // Change the value for `LangOpts.CurrentModule` temporarily to make the
     // module loader work properly.
     const_cast<LangOptions&>(getLangOpts()).CurrentModule = "";
-    Mod = getModuleLoader().loadModule(ModuleLoc, {ModuleNameLoc},
+    // First find the interface we need to import.
+    Interface = getModuleLoader().loadModule(ModuleLoc, {ModuleNameLoc},
                                              Module::AllVisible,
                                              /*IsInclusionDirective=*/false);
     const_cast<LangOptions&>(getLangOpts()).CurrentModule = ModuleName;
 
-    if (!Mod) {
+    if (!Interface) {
       Diag(ModuleLoc, diag::err_module_not_defined) << ModuleName;
       // Create an empty module interface unit for error recovery.
       Mod = Map.createModuleForInterfaceUnit(ModuleLoc, ModuleName);
+    } else {
+      // We found the interface, now create the implementation module
+      Mod = Map.createModuleForImplementationUnit(ModuleLoc, ModuleName);
     }
-
   } break;
 
   case ModuleDeclKind::PartitionImplementation:
     // Create an interface, but note that it is an implementation
     // unit.
     Mod = Map.createModuleForInterfaceUnit(ModuleLoc, ModuleName);
     Mod->Kind = Module::ModulePartitionImplementation;
     break;
   }
Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc,
   auto *TU = Context.getTranslationUnitDecl();
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ReachableWhenImported);
   TU->setLocalOwningModule(Mod);
 
   // We are in the module purview, but before any other (non import)
   // statements, so imports are allowed.
   ImportState = ModuleImportState::ImportAllowed;
 
-  // For an implementation, We already made an implicit import (its interface).
-  // Make and return the import decl to be added to the current TU.
-  if (MDK == ModuleDeclKind::Implementation) {
-    // Make the import decl for the interface.
-    ImportDecl *Import =
-        ImportDecl::Create(Context, CurContext, ModuleLoc, Mod, Path[0].second);
-    // and return it to be added.
+  getASTContext().setNamedModuleForCodeGen(Mod);
+
+  // We already potentially made an implicit import (in the case of a module
+  // implementation unit importing its interface).  Make this module visible
+  // and return the import decl to be added to the current TU.
+  if (Interface) {
+
+    VisibleModules.setVisible(Interface, ModuleLoc);
+
+    // Make the import decl for the interface in the impl module.
+    ImportDecl *Import = ImportDecl::Create(Context, CurContext, ModuleLoc,
+                                            Interface, Path[0].second);
+    CurContext->addDecl(Import);
+
+    // Sequence initialization of the imported module before that of the current
+    // module, if any.
+    if (!ModuleScopes.empty())
+      Context.addModuleInitializer(ModuleScopes.back().Module, Import);
+
+    Mod->Imports.insert(Interface); // As if we imported it.
+
+    // If we made an implicit import of the module interface, then return the
+    // imported module decl.
     return ConvertDeclToDeclGroup(Import);
   }
 
-  getASTContext().setNamedModuleForCodeGen(Mod);
-
   // FIXME: Create a ModuleDecl.
   return nullptr;
 }
 
 Sema::DeclGroupPtrTy
 Sema::ActOnPrivateModuleFragmentDecl(SourceLocation ModuleLoc,
                                      SourceLocation PrivateLoc) {
   // C++20 [basic.link]/2:
   //   A private-module-fragment shall appear only in a primary module
   //   interface unit.
   switch (ModuleScopes.empty() ? Module::ExplicitGlobalModuleFragment
                                : ModuleScopes.back().Module->Kind) {
   case Module::ModuleMapModule:
   case Module::ExplicitGlobalModuleFragment:
   case Module::ImplicitGlobalModuleFragment:
   case Module::ModulePartitionImplementation:
   case Module::ModulePartitionInterface:
   case Module::ModuleHeaderUnit:
     Diag(PrivateLoc, diag::err_private_module_fragment_not_module);
     return nullptr;
 
   case Module::PrivateModuleFragment:
     Diag(PrivateLoc, diag::err_private_module_fragment_redefined);
     Diag(ModuleScopes.back().BeginLoc, diag::note_previous_definition);
     return nullptr;
 
-  case Module::ModuleInterfaceUnit:
-    break;
-  }
-
-  if (!ModuleScopes.back().ModuleInterface) {
+  case Module::ModuleImplementationUnit:
     Diag(PrivateLoc, diag::err_private_module_fragment_not_module_interface);
     Diag(ModuleScopes.back().BeginLoc,
          diag::note_not_module_interface_add_export)
         << FixItHint::CreateInsertion(ModuleScopes.back().BeginLoc, "export ");
     return nullptr;
+
+  case Module::ModuleInterfaceUnit:
+    break;
   }
 
-  // FIXME: Check this isn't a module interface partition.
   // FIXME: Check that this translation unit does not import any partitions;
   // such imports would violate [basic.link]/2's "shall be the only module unit"
   // restriction.
 
   // We've finished the public fragment of the translation unit.
   ActOnEndOfTranslationUnitFragment(TUFragmentKind::Normal);
 
   auto &Map = PP.getHeaderSearchInfo().getModuleMap();

clang/lib/Serialization/ASTWriter.cpp

void ASTWriter::WriteSubmodules(Module *WritingModule) {
 
   // Write the abbreviations needed for the submodules block.
   using namespace llvm;
 
   auto Abbrev = std::make_shared<BitCodeAbbrev>();
   Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_DEFINITION));
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // ID
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Parent
-  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // Kind
+  Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 4)); // Kind
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsExplicit
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsSystem
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsExternC
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // InferSubmodules...
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // InferExplicit...
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // InferExportWild...
   Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // ConfigMacrosExh...

clang/test/CXX/module/basic/basic.def.odr/p4.cppm

 // RUN: rm -rf %t
 // RUN: mkdir %t
 // RUN: split-file %s %t
 //
 // RUN: %clang_cc1 -std=c++20 %t/Module.cppm -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %t/Module.cppm --implicit-check-not unused
 //
 // RUN: %clang_cc1 -std=c++20 %t/Module.cppm -triple %itanium_abi_triple -emit-module-interface -o %t/Module.pcm
-// RUN: %clang_cc1 -std=c++20 %t/module.cpp -triple %itanium_abi_triple -fmodule-file=%t/Module.pcm -emit-llvm -o - | FileCheck %t/module.cpp --implicit-check-not=unused --implicit-check-not=global_module
+// RUN: %clang_cc1 -std=c++20 %t/module.cpp -triple %itanium_abi_triple -fmodule-file=Module=%t/Module.pcm -emit-llvm -o - | FileCheck %t/module.cpp --implicit-check-not=unused --implicit-check-not=global_module
 //
-// RUN: %clang_cc1 -std=c++20 %t/user.cpp -triple %itanium_abi_triple -fmodule-file=%t/Module.pcm -emit-llvm -o - | FileCheck %t/user.cpp --implicit-check-not=unused --implicit-check-not=global_module
+// RUN: %clang_cc1 -std=c++20 %t/user.cpp -triple %itanium_abi_triple -fmodule-file=Module=%t/Module.pcm -emit-llvm -o - | FileCheck %t/user.cpp --implicit-check-not=unused --implicit-check-not=global_module
 
 //--- Module.cppm
 // CHECK-DAG: @extern_var_global_module = external {{(dso_local )?}}global
 // CHECK-DAG: @inline_var_global_module = linkonce_odr {{(dso_local )?}}global
 // CHECK-DAG: @_ZL24static_var_global_module = internal global
 // CHECK-DAG: @_ZL23const_var_global_module = internal constant
 //
 // With strong-ownership, the module is mangled into exported symbols
void use() {
   used_inline_exported();
   // CHECK: declare {{.*}}@_ZW6Module18noninline_exportedv
   noninline_exported();
 
   (void)&extern_var_exported;
   (void)&inline_var_exported;
   (void)&const_var_exported;
 
-  // CHECK: define {{.*}}@_ZL26used_static_module_linkagev
-  used_static_module_linkage();
-
   // CHECK: define linkonce_odr {{.*}}@_ZW6Module26used_inline_module_linkagev
   used_inline_module_linkage();
 
   // CHECK: declare {{.*}}@_ZW6Module24noninline_module_linkagev
   noninline_module_linkage();
 
   (void)&extern_var_module_linkage;
   (void)&inline_var_module_linkage;
+
+  // FIXME: Issue #61427 Internal-linkage declarations in the interface TU
+  // should not be not visible here.
   (void)&static_var_module_linkage; // FIXME: Should not be visible here.
-  (void)&const_var_module_linkage;
+
+  (void)&const_var_module_linkage; // FIXME: will be visible after P2788R0
 }
 
 //--- user.cpp
 
 // CHECK-DAG: @_ZW6Module19extern_var_exported = external {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6Module19inline_var_exported = linkonce_odr {{(dso_local )?}}global
 // CHECK-DAG: @_ZW6Module18const_var_exported = available_externally {{(dso_local )?}}constant i32 3
 
 import Module;
 
 void use() {
   // CHECK: define linkonce_odr {{.*}}@_ZW6Module20used_inline_exportedv
   used_inline_exported();
   // CHECK: declare {{.*}}@_ZW6Module18noninline_exportedv
   noninline_exported();
 
   (void)&extern_var_exported;
   (void)&inline_var_exported;
   (void)&const_var_exported;
 
+  // Internal-linkage declarations are not visible here.
   // Module-linkage declarations are not visible here.
 }

clang/test/CXX/module/basic/basic.link/p2.cppm

void use() {
   (void)module_linkage_class{};
   (void)internal_linkage_class{};
   (void)external_linkage_var;
   (void)module_linkage_var;
   (void)internal_linkage_var;
 }
 
 //--- M.cpp
-// expected-no-diagnostics
+
 module M;
 
-// FIXME: Use of internal linkage entities should be rejected.
 void use_from_module_impl() {
   external_linkage_fn();
   module_linkage_fn();
-  internal_linkage_fn();
+  internal_linkage_fn(); // expected-error {{no matching function for call to 'internal_linkage_fn'}}
   (void)external_linkage_class{};
   (void)module_linkage_class{};
-  (void)internal_linkage_class{};
   (void)external_linkage_var;
   (void)module_linkage_var;
+
+  // FIXME: Issue #61427 Internal-linkage declarations in the interface TU
+  // should not be not visible here.
+  (void)internal_linkage_class{};
   (void)internal_linkage_var;
 }
 
 //--- user.cpp
 import M;
 
 void use_from_module_impl() {
   external_linkage_fn();

clang/test/CodeGenCXX/module-intializer.cpp

 // RUN:  -o - | FileCheck %s --check-prefix=CHECK-O
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M-part.cpp \
 // RUN:    -emit-module-interface -o M-part.pcm
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M-part.pcm -S \
 // RUN: -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-P
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M.cpp \
-// RUN: -fmodule-file=N.pcm -fmodule-file=O.pcm -fmodule-file=M-part.pcm \
+// RUN: -fmodule-file=N=N.pcm -fmodule-file=O=O.pcm -fmodule-file=M:Part=M-part.pcm \
 // RUN:    -emit-module-interface -o M.pcm
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M.pcm -S -emit-llvm \
 // RUN:  -o - | FileCheck %s --check-prefix=CHECK-M
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 useM.cpp \
-// RUN: -fmodule-file=M.pcm -S -emit-llvm  -o - \
+// RUN: -fmodule-file=M=M.pcm -S -emit-llvm  -o - \
 // RUN: | FileCheck %s --check-prefix=CHECK-USE
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M-impl.cpp \
-// RUN: -fmodule-file=M.pcm -S -emit-llvm  -o - \
+// RUN: -fmodule-file=M=M.pcm -S -emit-llvm  -o - \
 // RUN: | FileCheck %s --check-prefix=CHECK-IMPL
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 N.cpp -S -emit-llvm \
 // RUN:   -o - | FileCheck %s --check-prefix=CHECK-N
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 O.cpp -S -emit-llvm \
 // RUN:   -o - | FileCheck %s --check-prefix=CHECK-O
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M-part.cpp -S -emit-llvm \
 // RUN:   -o - | FileCheck %s --check-prefix=CHECK-P
 
 // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M.cpp \
-// RUN:   -fmodule-file=N.pcm -fmodule-file=O.pcm -fmodule-file=M-part.pcm \
+// RUN:   -fmodule-file=N.pcm -fmodule-file=O=O.pcm -fmodule-file=M:Part=M-part.pcm \
 // RUN:   -S -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-M
 
 //--- N-h.h
 
 struct Oink {
   Oink(){};
 };