Modular Codegen: Emit inline asm into users, not modular objects

[dblaikie]

Summary

The best semantics I can come up with (based on discussions with
Richard) for inline asm and namespace scope internal linkage variables
in modular headers is to emit them into all users and never into modular
objects.

The reason for this is that such asm might create important symbols
(like the iostreams global initializer) that can only be produced if the
submodule is used (doesn't strictly apply to inline asm, as such - which
is currently emitted if the module is used at all (regardless of
submodules) I think). This does mean that if any modular code does use
these entities it will be ill formed/NDR (link error, generally) for
now. It could be diagnosed at parse/module-creation time.

This change has a bit of refactoring to allow this to work for inline
asm (& to work more efficiently for internal linkage namespace-scope
variables). Making the MODULAR_CODEGEN_DECLS and
EAGERLY_DESERIALIZED_DECLS distinct. Not only does this allow the
modular codegen not to needlessly deserialize and then ignore the
eagerly deserialized decls, it also allows there to be eagerly
deserialized decls that modular codegen never sees - such as inline asm.

(if this isn't the right design - if the absence of an entry in
eagerly/modular codegen shouldn't produce different behavior (ie: it
should only be an optimization) happy to make this work
differently/harder by making sure inline asm can be filtered, even if it
were loaded)

This change does a little work to preserve a scenario that it probably
doesn't need to: 'classic' AST codegen. There's a few tests
(test/Frontend/ast-*) that verify that the codegen of a serialized AST
file (without -fmodules-codegen) produces basically the same code as if
the AST hadn't been serialized. Is this important? Should we kill off
those tests & not worry about it? (this behavior is preserved by still
respecting the EAGERLY_DESERIALIZED_DECLS when deserializing an AST file
if it doesn't contain MODULAR_CODEGEN_DECLS).

[dblaikie]

Oh, one of the reasons I was sort of compelled to keep the 'classic' AST file compilation working was that when looking at the change history of the test cases exercising it I found that some changes had been motivated by a filed LLVM bug: https://llvm.org/bugs/show_bug.cgi?id=15377 - so I guess someone might've been trying to use it at some point?

[dblaikie]

+rsmith we chatted about a bit of this offline & some things came out of it, not entirely resolved:

1. Is this the right thing for inline asm or similar things (static functions? static variables?) - I was mirroring the choice already made for static variables (motivated by the iostreams initializer) sort of broadening that idea to "you can put internal linkage things in headers so long as you don't actually violate the ODR by referring to them in linkage-having entities in the header". But that may be impractically aggressive. Practically speaking maybe static locals get a special exemption and everything else goes everywhere it's used, modular object or not?

2. The refactoring of DeclMustBeEmitted to take an (in)out parameter seemed good, but there was some discussion around whether it should instead return a more complex value rather than an optional inout parameter. Did you have any further thoughts on that? I think we talked about it possibly being a bit complicated for existing callers because they want to treat two different states as "false" (both Never and Always map to "it doesn't /have/ to be emitted").

include/clang/AST/ASTContext.h

public:
   GVALinkage GetGVALinkageForVariable(const VarDecl *VD);
 
   /// \brief Determines if the decl can be CodeGen'ed or deserialized from PCH
   /// lazily, only when used; this is only relevant for function or file scoped
   /// var definitions.
   ///
   /// \returns true if the function/var must be CodeGen'ed/deserialized even if
   /// it is not used.
-  bool DeclMustBeEmitted(const Decl *D, bool ForModularCodegen = false);
+  bool DeclMustBeEmitted(const Decl *D, bool *ForModuleFile = nullptr);
 
   const CXXConstructorDecl *
   getCopyConstructorForExceptionObject(CXXRecordDecl *RD);
 
   void addCopyConstructorForExceptionObject(CXXRecordDecl *RD,
                                             CXXConstructorDecl *CD);
 
   void addTypedefNameForUnnamedTagDecl(TagDecl *TD, TypedefNameDecl *TND);

include/clang/Serialization/ASTReader.h

private:
   /// \brief The IDs of all declarations that fulfill the criteria of
   /// "interesting" decls.
   ///
   /// This contains the data loaded from all EAGERLY_DESERIALIZED_DECLS blocks
   /// in the chain. The referenced declarations are deserialized and passed to
   /// the consumer eagerly.
   SmallVector<uint64_t, 16> EagerlyDeserializedDecls;
 
-  SmallVector<uint64_t, 16> ModularCodegenDecls;
+  Optional<SmallVector<uint64_t, 16>> ModularCodegenDecls;
 
   /// \brief The IDs of all tentative definitions stored in the chain.
   ///
   /// Sema keeps track of all tentative definitions in a TU because it has to
   /// complete them and pass them on to CodeGen. Thus, tentative definitions in
   /// the PCH chain must be eagerly deserialized.
   SmallVector<uint64_t, 16> TentativeDefinitions;
 

lib/AST/ASTContext.cpp

static GVALinkage basicGVALinkageForVariable(const ASTContext &Context,
   llvm_unreachable("Invalid Linkage!");
 }
 
 GVALinkage ASTContext::GetGVALinkageForVariable(const VarDecl *VD) {
   return adjustGVALinkageForAttributes(
       *this, basicGVALinkageForVariable(*this, VD), VD);
 }
 
-bool ASTContext::DeclMustBeEmitted(const Decl *D, bool ForModularCodegen) {
+bool ASTContext::DeclMustBeEmitted(const Decl *D, bool *ForModuleFile) {
+  assert(!ForModuleFile || !*ForModuleFile);
   if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
     if (!VD->isFileVarDecl())
       return false;
     // Global named register variables (GNU extension) are never emitted.
     if (VD->getStorageClass() == SC_Register)
       return false;
     if (VD->getDescribedVarTemplate() ||
         isa<VarTemplatePartialSpecializationDecl>(VD))
if (getTargetInfo().getCXXABI().canKeyFunctionBeInline()) {
           if (KeyFunc && KeyFunc->getCanonicalDecl() == MD->getCanonicalDecl())
             return true;
         }
       }
     }
 
     GVALinkage Linkage = GetGVALinkageForFunction(FD);
 
-    if (Linkage == GVA_DiscardableODR && ForModularCodegen)
+    if ((Linkage == GVA_DiscardableODR || Linkage == GVA_StrongExternal) &&
+        ForModuleFile) {
+      *ForModuleFile = true;
       return true;
+    }
 
     // static, static inline, always_inline, and extern inline functions can
     // always be deferred.  Normal inline functions can be deferred in C99/C++.
     // Implicit template instantiations can also be deferred in C++.
     return !isDiscardableGVALinkage(Linkage);
   }
   
   const VarDecl *VD = cast<VarDecl>(D);

lib/Serialization/ASTReader.cpp

case EAGERLY_DESERIALIZED_DECLS:
       // about "interesting" decls (for instance, if we're building a module).
       for (unsigned I = 0, N = Record.size(); I != N; ++I)
         EagerlyDeserializedDecls.push_back(getGlobalDeclID(F, Record[I]));
       break;
 
     case MODULAR_CODEGEN_DECLS:
       // FIXME: Skip reading this record if our ASTConsumer doesn't care about
       // them (ie: if we're not codegenerating this module).
-      if (F.Kind == MK_MainFile)
+      if (F.Kind == MK_MainFile) {
+        ModularCodegenDecls.emplace();
         for (unsigned I = 0, N = Record.size(); I != N; ++I)
-          EagerlyDeserializedDecls.push_back(getGlobalDeclID(F, Record[I]));
+          ModularCodegenDecls->push_back(getGlobalDeclID(F, Record[I]));
+      }
       break;
 
     case SPECIAL_TYPES:
       if (SpecialTypes.empty()) {
         for (unsigned I = 0, N = Record.size(); I != N; ++I)
           SpecialTypes.push_back(getGlobalTypeID(F, Record[I]));
         break;
       }
void ASTReader::PassInterestingDeclsToConsumer() {
 
   // Guard variable to avoid recursively redoing the process of passing
   // decls to consumer.
   SaveAndRestore<bool> GuardPassingDeclsToConsumer(PassingDeclsToConsumer,
                                                    true);
 
   // Ensure that we've loaded all potentially-interesting declarations
   // that need to be eagerly loaded.
-  for (auto ID : EagerlyDeserializedDecls)
+  auto GetDecls = [&](const SmallVector<uint64_t, 16> &V) {
+    for (auto ID : V)
       GetDecl(ID);
+  };
+  if (ModularCodegenDecls)
+    GetDecls(*ModularCodegenDecls);
+  else
+    GetDecls(EagerlyDeserializedDecls);
+  ModularCodegenDecls = None;
   EagerlyDeserializedDecls.clear();
 
   while (!InterestingDecls.empty()) {
     Decl *D = InterestingDecls.front();
     InterestingDecls.pop_front();
 
     PassInterestingDeclToConsumer(D);
   }

lib/Serialization/ASTWriterDecl.cpp

 /// code generation requirements, if other clients have a different notion of
 /// what is "required" then we may have to consider an alternate scheme where
 /// clients can iterate over the top-level decls and get information on them,
 /// without necessary deserializing them. We could explicitly require such
 /// clients to use a separate API call to "realize" the decl. This should be
 /// relatively painless since they would presumably only do it for top-level
 /// decls.
 static bool isRequiredDecl(const Decl *D, ASTContext &Context,
-                           bool WritingModule, bool ModularCode) {
+                           bool WritingModule, bool *ForModuleFile) {
+  assert(!ForModuleFile || !*ForModuleFile);
   // An ObjCMethodDecl is never considered as "required" because its
   // implementation container always is.
 
   // File scoped assembly or obj-c or OMP declare target implementation must be
   // seen.
   if (isa<FileScopeAsmDecl>(D) || isa<ObjCImplDecl>(D) ||
       D->hasAttr<OMPDeclareTargetDeclAttr>())
     return true;
 
   if (WritingModule && (isa<VarDecl>(D) || isa<ImportDecl>(D))) {
     // These declarations are part of the module initializer, and are emitted
     // if and when the module is imported, rather than being emitted eagerly.
     return false;
   }
 
-  return Context.DeclMustBeEmitted(D, ModularCode);
+  return Context.DeclMustBeEmitted(D, ForModuleFile);
 }
 
 void ASTWriter::WriteDecl(ASTContext &Context, Decl *D) {
   // Determine the ID for this declaration.
   serialization::DeclID ID;
   assert(!D->isFromASTFile() && "should not be emitting imported decl");
   serialization::DeclID &IDR = DeclIDs[D];
   if (IDR == 0)
void ASTWriter::WriteDecl(ASTContext &Context, Decl *D) {
   }
 
   SourceManager &SM = Context.getSourceManager();
   if (Loc.isValid() && SM.isLocalSourceLocation(Loc))
     associateDeclWithFile(D, ID);
 
   // Note declarations that should be deserialized eagerly so that we can add
   // them to a record in the AST file later.
-  if (isRequiredDecl(D, Context, WritingModule, false))
-    EagerlyDeserializedDecls.push_back(ID);
-  else if (Context.getLangOpts().ModularCodegen && WritingModule &&
-           isRequiredDecl(D, Context, true, true))
-    ModularCodegenDecls.push_back(ID);
+  bool ForModuleFile = false;
+  if (isRequiredDecl(D, Context, WritingModule,
+                     Context.getLangOpts().ModularCodegen && WritingModule
+                         ? &ForModuleFile
+                         : nullptr))
+    (ForModuleFile ? ModularCodegenDecls : EagerlyDeserializedDecls)
+        .push_back(ID);
 }
 
 void ASTRecordWriter::AddFunctionDefinition(const FunctionDecl *FD) {
   // Switch case IDs are per function body.
   Writer->ClearSwitchCaseIDs();
 
   assert(FD->doesThisDeclarationHaveABody());
   if (auto *CD = dyn_cast<CXXConstructorDecl>(FD)) {
     Record->push_back(CD->getNumCtorInitializers());
     if (CD->getNumCtorInitializers())
       AddCXXCtorInitializers(
           llvm::makeArrayRef(CD->init_begin(), CD->init_end()));
   }
   AddStmt(FD->getBody());
 }

test/CodeGenCXX/modules-ts.cppm

 // RUN: %clang_cc1 -fmodules-ts -std=c++1z -triple=x86_64-linux-gnu -fmodules-codegen -emit-module-interface %s -o %t.pcm
 // RUN: %clang_cc1 -fmodules-ts -std=c++1z -triple=x86_64-linux-gnu %t.pcm -emit-llvm -o - | FileCheck %s
 
 module FooBar;
 
+// CHECK-LABEL: define weak_odr void @_Z2f2v(
+inline void f2() { }
+
 export {
   // CHECK-LABEL: define i32 @_Z1fv(
   int f() { return 0; }
 }
 
-// CHECK-LABEL: define weak_odr void @_Z2f2v(
-inline void f2() { }
-
 // FIXME: Emit global variables and their initializers with this TU.
 // Emit an initialization function that other TUs can call, with guard variable.
 
 // FIXME: Mangle non-exported symbols so they don't collide with
 // non-exported symbols from other modules?
 
 // FIXME: Formally-internal-linkage symbols that are used from an exported
 // symbol need a mangled name and external linkage.
 
 // FIXME: const-qualified variables don't have implicit internal linkage when owned by a module.

test/Modules/Inputs/codegen/foo.h

+asm(".byte 42");
 void f1(int &);
 static void f2() {}
 inline void foo() {
   static int i;
   f1(i);
   f2();
 }
 inline void foo2() {
 }
 void foo_ext() {}

test/Modules/codegen.test

 RUN: rm -rf %t
 REQUIRES: x86-registered-target
 
 RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++ -fmodules -emit-module -fmodule-name=foo %S/Inputs/codegen/foo.modulemap -o %t/foo.pcm
 RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++ -fmodules -emit-module -fmodule-name=bar %S/Inputs/codegen/bar.modulemap -o %t/bar.pcm -fmodule-file=%t/foo.pcm
 
 RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %t/foo.pcm | FileCheck --check-prefix=FOO %s
 RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %t/bar.pcm -fmodule-file=%t/foo.pcm | FileCheck --check-prefix=BAR-CMN --check-prefix=BAR %s
 RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -fmodules -fmodule-file=%t/foo.pcm -fmodule-file=%t/bar.pcm %S/Inputs/codegen/use.cpp | FileCheck --check-prefix=USE-CMN --check-prefix=USE %s
 
 RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-llvm-passes %t/foo.pcm | FileCheck --check-prefix=FOO %s
 RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-llvm-passes %t/bar.pcm -fmodule-file=%t/foo.pcm | FileCheck --check-prefix=BAR-CMN --check-prefix=BAR-OPT %s
 RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-llvm-passes -fmodules -fmodule-file=%t/foo.pcm -fmodule-file=%t/bar.pcm %S/Inputs/codegen/use.cpp | FileCheck --check-prefix=USE-CMN --check-prefix=USE-OPT %s
 
-FOO-NOT: comdat
+FOO-NOT: {{comdat|module asm}}
 FOO: $_Z3foov = comdat any
 FOO: $_Z4foo2v = comdat any
 FOO: $_ZZ3foovE1i = comdat any
 FOO: @_ZZ3foovE1i = linkonce_odr global i32 0, comdat
 FOO-NOT: {{comdat|define|declare}}
-FOO: define void @_Z7foo_extv()
-FOO-NOT: {{define|declare}}
 FOO: define weak_odr void @_Z3foov() #{{[0-9]+}} comdat
 FOO-NOT: {{define|declare}}
 FOO: declare void @_Z2f1Ri(i32*
 FOO-NOT: {{define|declare}}
 
 FIXME: this internal function should be weak_odr, comdat, and with a new mangling
 FOO: define internal void @_ZL2f2v() #{{[0-9]+}}
 FOO-NOT: {{define|declare}}
 
 FOO: define weak_odr void @_Z4foo2v() #{{[0-9]+}} comdat
 FOO-NOT: {{define|declare}}
+FOO: define void @_Z7foo_extv()
+FOO-NOT: {{define|declare}}
 
 
-BAR-CMN-NOT: comdat
+BAR-CMN-NOT: {{comdat|module asm}}
 BAR-CMN: $_Z3barv = comdat any
+BAR-CMN-NOT: comdat
+BAR-OPT: $_ZZ3foovE1i = comdat any
+BAR-OPT-NOT: comdat
 BAR-OPT: @_ZZ3foovE1i = linkonce_odr global i32 0, comdat
-BAR-CMN-NOT: {{comdat|define|declare}}
+BAR-CMN-NOT: {{define|declare}}
 BAR-CMN: define weak_odr void @_Z3barv() #{{[0-9]+}} comdat
 BAR-CMN-NOT: {{define|declare}}
 BAR: declare void @_Z3foov()
+BAR-NOT: {{define|declare}}
 Include all the available_externally definitions required for bar (foo -> f2)
 BAR-OPT: define available_externally void @_Z3foov()
-BAR-CMN-NOT: {{define|declare}}
+BAR-OPT-NOT: {{define|declare}}
 BAR-OPT: declare void @_Z2f1Ri(i32*
 BAR-OPT-NOT: {{define|declare}}
 BAR-OPT: define available_externally void @_ZL2f2v()
 BAR-OPT-NOT: {{define|declare}}
 
 
+USE-CMN-NOT: {{comdat|module asm}}
+USE-CMN: module asm ".byte 42"
+USE-CMN-NOT: {{comdat|module asm}}
+USE-OPT: $_ZZ3foovE1i = comdat any
+USE-OPT-CMN-NOT: {{comdat}}
 USE-OPT: @_ZZ3foovE1i = linkonce_odr global i32 0, comdat
-USE-CMN-NOT: {{comdat|define|declare}}
+USE-CMN-NOT: {{define|declare}}
 USE-CMN: define i32 @main()
 USE-CMN-NOT: {{define|declare}}
 USE: declare void @_Z3barv()
 Include all the available_externally definitions required for main (bar -> foo -> f2)
 USE-OPT: define available_externally void @_Z3barv()
 USE-CMN-NOT: {{define|declare}}
 USE-OPT: define available_externally void @_Z3foov()
 USE-OPT-NOT: {{define|declare}}
 USE-OPT: declare void @_Z2f1Ri(i32*
 USE-OPT-NOT: {{define|declare}}
 USE-OPT: define available_externally void @_ZL2f2v()
 USE-OPT-NOT: {{define|declare}}