[C++20] [Modules] Introduce thin BMI #71622
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesClose #71034 This patch introduces thin BMI, which doesn't contain the definitions of functions and variables if its definitions won't contribute to the ABI. Testing is a big part of the patch. We want to make sure the thin BMI contains the same behavior with the existing and relatively stable fatBMI. This is pretty helpful for further reduction. For user interfeaces, this patch introduces The design is helpful to use thin BMI in two phase compilations too. With thin BMI, In two phase compilations, we'll generate 2 BMIs, one thin BMI for being used by consumers, one fat BMI for compiling itself to object files. Maybe it sounds confusing to have 2 BMIs for one module unit. But only the thin BMI will be the BMI we're talking about generally and the fat BMI is only visible by the module unit itself. With one phase compilation, we may find the behavior of But, of course, in the end of the day, we want the consumers to use the thin BMI only. When that day comes, the Another design choice is to reuse The roadmap for thin BMI in my mind is: (1) In clang18, release thin BMI and mark it as experimental. Also encourage users and build systems to try this new mode. (2) In clang19 or clang20 (based on the issue feedbacks), remove the experimental mark for thin BMI and mark fat BMI as deprecated to be used by consumers. CC: @mathstuf Patch is 113.03 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71622.diff 112 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 676f1a62b49dd0d..aad67a9f4c7da01 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -158,6 +158,8 @@ def err_drv_invalid_output_with_multiple_archs : Error<
def err_drv_no_input_files : Error<"no input files">;
def err_drv_output_argument_with_multiple_files : Error<
"cannot specify -o when generating multiple output files">;
+def err_drv_thin_bmi_output_argument_with_multiple_files : Error <
+ "cannot specify -fthinBMI-output when generating multiple module files">;
def err_drv_out_file_argument_with_multiple_sources : Error<
"cannot specify '%0%1' when compiling multiple source files">;
def err_no_external_assembler : Error<
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 36052511203f65c..12785f280183e03 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2915,6 +2915,11 @@ def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption]>,
Visibility<[ClangOption, CC1Option]>,
HelpText<"Save intermediate module file results when compiling a standard C++ module unit.">;
+def fthinBMI_output_EQ : Joined<["-"], "fthinBMI-output=">, Group<f_Group>,
+ HelpText<"Specify the output path for the thin BMI for C++20 Named modules">,
+ Visibility<[ClangOption, CC1Option, CLOption, DXCOption]>,
+ MarshallingInfoString<FrontendOpts<"ThinBMIPath">>;
+
def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, Group<i_Group>,
Visibility<[ClangOption, CC1Option]>, MetaVarName<"<seconds>">,
HelpText<"Specify the interval (in seconds) between attempts to prune the module cache">,
@@ -7223,7 +7228,9 @@ def ast_view : Flag<["-"], "ast-view">,
def emit_module : Flag<["-"], "emit-module">,
HelpText<"Generate pre-compiled module file from a module map">;
def emit_module_interface : Flag<["-"], "emit-module-interface">,
- HelpText<"Generate pre-compiled module file from a C++ module interface">;
+ HelpText<"Generate pre-compiled module file from a standard C++ module interface unit">;
+def emit_thin_module_interface : Flag<["-"], "emit-thin-module-interface">,
+ HelpText<"Generate reduced prebuilt module interface from a standard C++ module interface unit">;
def emit_header_unit : Flag<["-"], "emit-header-unit">,
HelpText<"Generate C++20 header units from header files">;
def emit_pch : Flag<["-"], "emit-pch">,
diff --git a/clang/include/clang/Frontend/FrontendActions.h b/clang/include/clang/Frontend/FrontendActions.h
index 3940e00eeb8dba7..c419deb80034fae 100644
--- a/clang/include/clang/Frontend/FrontendActions.h
+++ b/clang/include/clang/Frontend/FrontendActions.h
@@ -118,6 +118,9 @@ class GenerateModuleAction : public ASTFrontendAction {
CreateOutputFile(CompilerInstance &CI, StringRef InFile) = 0;
protected:
+ std::vector<std::unique_ptr<ASTConsumer>>
+ CreateMultiplexConsumer(CompilerInstance &CI, StringRef InFile);
+
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
StringRef InFile) override;
@@ -147,14 +150,27 @@ class GenerateModuleFromModuleMapAction : public GenerateModuleAction {
CreateOutputFile(CompilerInstance &CI, StringRef InFile) override;
};
+/// Generates fatBMI (which contains full information to generate the object
+/// files) for C++20 Named Modules. Also generates the thin BMI (only contains
+/// necessary information for importers) if `-fthinBMI-output=`.
class GenerateModuleInterfaceAction : public GenerateModuleAction {
-private:
+protected:
bool BeginSourceFileAction(CompilerInstance &CI) override;
+ std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+ StringRef InFile) override;
+
std::unique_ptr<raw_pwrite_stream>
CreateOutputFile(CompilerInstance &CI, StringRef InFile) override;
};
+/// Only generates the thin BMI. This action is mainly used by tests.
+class GenerateThinModuleInterfaceAction : public GenerateModuleInterfaceAction {
+private:
+ std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+ StringRef InFile) override;
+};
+
class GenerateHeaderUnitAction : public GenerateModuleAction {
private:
diff --git a/clang/include/clang/Frontend/FrontendOptions.h b/clang/include/clang/Frontend/FrontendOptions.h
index 53a8681cfdbba04..719d4ca81336116 100644
--- a/clang/include/clang/Frontend/FrontendOptions.h
+++ b/clang/include/clang/Frontend/FrontendOptions.h
@@ -85,9 +85,13 @@ enum ActionKind {
/// Generate pre-compiled module from a module map.
GenerateModule,
- /// Generate pre-compiled module from a C++ module interface file.
+ /// Generate pre-compiled module from a standard C++ module interface unit.
GenerateModuleInterface,
+ /// Generate reduced module interface for a standard C++ module interface
+ /// unit.
+ GenerateThinModuleInterface,
+
/// Generate a C++20 header unit module from a header file.
GenerateHeaderUnit,
@@ -549,6 +553,9 @@ class FrontendOptions {
/// Path which stores the output files for -ftime-trace
std::string TimeTracePath;
+ /// Path to the thin BMI for -fthinbmi-output=
+ std::string ThinBMIPath;
+
public:
FrontendOptions()
: DisableFree(false), RelocatablePCH(false), ShowHelp(false),
diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h
index 3019bbc2ddc9cc7..be2d0e227177c34 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -166,6 +166,10 @@ class ASTWriter : public ASTDeserializationListener,
/// Indicates that the AST contained compiler errors.
bool ASTHasCompilerErrors = false;
+ /// Indicates that we're going to generate the reduced BMI for C++20
+ /// named modules.
+ bool GeneratingThinBMI = false;
+
/// Mapping from input file entries to the index into the
/// offset table where information about that input file is stored.
llvm::DenseMap<const FileEntry *, uint32_t> InputFileIDs;
@@ -582,7 +586,8 @@ class ASTWriter : public ASTDeserializationListener,
ASTWriter(llvm::BitstreamWriter &Stream, SmallVectorImpl<char> &Buffer,
InMemoryModuleCache &ModuleCache,
ArrayRef<std::shared_ptr<ModuleFileExtension>> Extensions,
- bool IncludeTimestamps = true, bool BuildingImplicitModule = false);
+ bool IncludeTimestamps = true, bool BuildingImplicitModule = false,
+ bool GeneratingThinBMI = false);
~ASTWriter() override;
ASTContext &getASTContext() const {
@@ -813,6 +818,13 @@ class PCHGenerator : public SemaConsumer {
const ASTWriter &getWriter() const { return Writer; }
SmallVectorImpl<char> &getPCH() const { return Buffer->Data; }
+ bool isComplete() const { return Buffer->IsComplete; }
+ PCHBuffer *getBufferPtr() { return Buffer.get(); }
+ StringRef getOutputFile() const { return OutputFile; }
+ DiagnosticsEngine &getDiagnostics() const {
+ return SemaPtr->getDiagnostics();
+ }
+
public:
PCHGenerator(const Preprocessor &PP, InMemoryModuleCache &ModuleCache,
StringRef OutputFile, StringRef isysroot,
@@ -820,7 +832,8 @@ class PCHGenerator : public SemaConsumer {
ArrayRef<std::shared_ptr<ModuleFileExtension>> Extensions,
bool AllowASTWithErrors = false, bool IncludeTimestamps = true,
bool BuildingImplicitModule = false,
- bool ShouldCacheASTInMemory = false);
+ bool ShouldCacheASTInMemory = false,
+ bool GeneratingThinBMI = false);
~PCHGenerator() override;
void InitializeSema(Sema &S) override { SemaPtr = &S; }
@@ -830,6 +843,20 @@ class PCHGenerator : public SemaConsumer {
bool hasEmittedPCH() const { return Buffer->IsComplete; }
};
+class ThinBMIGenerator : public PCHGenerator {
+public:
+ ThinBMIGenerator(const Preprocessor &PP, InMemoryModuleCache &ModuleCache,
+ StringRef OutputFile, std::shared_ptr<PCHBuffer> Buffer,
+ ArrayRef<std::shared_ptr<ModuleFileExtension>> Extensions,
+ bool IncludeTimestamps);
+
+ void HandleTranslationUnit(ASTContext &Ctx) override;
+};
+
+/// If the definition may impact the ABI. If yes, we're allowed to eliminate
+/// the definition of D in thin BMI.
+bool MayDefAffectABI(const Decl *D);
+
/// A simple helper class to pack several bits in order into (a) 32 bit
/// integer(s).
class BitsPacker {
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 6f5ff8141032677..76def412d12552c 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -4086,6 +4086,13 @@ void Driver::BuildActions(Compilation &C, DerivedArgList &Args,
}
}
+ // Diagnose misuse of -fthinBMI-output. It should be an error if we specify
+ // -fthinBMI-output with multiple precompilation jobs. Here we didn't check if
+ // there are multiple module units in the inputs.
+ if (C.getArgs().getLastArg(options::OPT_fthinBMI_output_EQ) &&
+ Inputs.size() > 1)
+ Diag(clang::diag::err_drv_thin_bmi_output_argument_with_multiple_files);
+
handleArguments(C, Args, Inputs, Actions);
bool UseNewOffloadingDriver =
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 22f992166ded6c0..5bb945be78dcb41 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3940,6 +3940,8 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
Args.ClaimAllArgs(options::OPT_fmodule_output);
Args.ClaimAllArgs(options::OPT_fmodule_output_EQ);
+ Args.AddLastArg(CmdArgs, options::OPT_fthinBMI_output_EQ);
+
return HaveModules;
}
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 637c6a35af6532b..b6245d0dddb1052 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -2554,6 +2554,7 @@ static const auto &getFrontendActionTable() {
{frontend::GenerateModule, OPT_emit_module},
{frontend::GenerateModuleInterface, OPT_emit_module_interface},
+ {frontend::GenerateThinModuleInterface, OPT_emit_thin_module_interface},
{frontend::GenerateHeaderUnit, OPT_emit_header_unit},
{frontend::GeneratePCH, OPT_emit_pch},
{frontend::GenerateInterfaceStubs, OPT_emit_interface_stubs},
@@ -4236,6 +4237,7 @@ static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
case frontend::FixIt:
case frontend::GenerateModule:
case frontend::GenerateModuleInterface:
+ case frontend::GenerateThinModuleInterface:
case frontend::GenerateHeaderUnit:
case frontend::GeneratePCH:
case frontend::GenerateInterfaceStubs:
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index 2afcf1cf9f68c81..1c66022993ba216 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -184,12 +184,12 @@ bool GeneratePCHAction::BeginSourceFileAction(CompilerInstance &CI) {
return true;
}
-std::unique_ptr<ASTConsumer>
-GenerateModuleAction::CreateASTConsumer(CompilerInstance &CI,
- StringRef InFile) {
+std::vector<std::unique_ptr<ASTConsumer>>
+GenerateModuleAction::CreateMultiplexConsumer(CompilerInstance &CI,
+ StringRef InFile) {
std::unique_ptr<raw_pwrite_stream> OS = CreateOutputFile(CI, InFile);
if (!OS)
- return nullptr;
+ return {};
std::string OutputFile = CI.getFrontendOpts().OutputFile;
std::string Sysroot;
@@ -210,6 +210,17 @@ GenerateModuleAction::CreateASTConsumer(CompilerInstance &CI,
+CI.getFrontendOpts().BuildingImplicitModule));
Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
CI, std::string(InFile), OutputFile, std::move(OS), Buffer));
+ return std::move(Consumers);
+}
+
+std::unique_ptr<ASTConsumer>
+GenerateModuleAction::CreateASTConsumer(CompilerInstance &CI,
+ StringRef InFile) {
+ std::vector<std::unique_ptr<ASTConsumer>> Consumers =
+ CreateMultiplexConsumer(CI, InFile);
+ if (Consumers.empty())
+ return nullptr;
+
return std::make_unique<MultiplexConsumer>(std::move(Consumers));
}
@@ -264,6 +275,38 @@ GenerateModuleInterfaceAction::CreateOutputFile(CompilerInstance &CI,
return CI.createDefaultOutputFile(/*Binary=*/true, InFile, "pcm");
}
+static std::unique_ptr<ASTConsumer>
+CreateThinBMIGenerator(CompilerInstance &CI, StringRef OutputFile) {
+ auto Buffer = std::make_shared<PCHBuffer>();
+ // We don't have modules extension in Named mdoules.
+ llvm::SmallVector<std::shared_ptr<ModuleFileExtension>, 1> ModulesExtensions;
+ return std::make_unique<ThinBMIGenerator>(
+ CI.getPreprocessor(), CI.getModuleCache(), OutputFile, Buffer,
+ ModulesExtensions,
+ /*IncludeTimestamps=*/+CI.getFrontendOpts().IncludeTimestamps);
+}
+
+std::unique_ptr<ASTConsumer>
+GenerateModuleInterfaceAction::CreateASTConsumer(CompilerInstance &CI,
+ StringRef InFile) {
+ std::vector<std::unique_ptr<ASTConsumer>> Consumers =
+ CreateMultiplexConsumer(CI, InFile);
+ if (Consumers.empty())
+ return nullptr;
+
+ if (!CI.getFrontendOpts().ThinBMIPath.empty())
+ Consumers.push_back(
+ CreateThinBMIGenerator(CI, CI.getFrontendOpts().ThinBMIPath));
+
+ return std::make_unique<MultiplexConsumer>(std::move(Consumers));
+}
+
+std::unique_ptr<ASTConsumer>
+GenerateThinModuleInterfaceAction::CreateASTConsumer(CompilerInstance &CI,
+ StringRef InFile) {
+ return CreateThinBMIGenerator(CI, CI.getFrontendOpts().OutputFile);
+}
+
bool GenerateHeaderUnitAction::BeginSourceFileAction(CompilerInstance &CI) {
if (!CI.getLangOpts().CPlusPlusModules) {
CI.getDiagnostics().Report(diag::err_module_interface_requires_cpp_modules);
@@ -830,7 +873,6 @@ void DumpModuleInfoAction::ExecuteAction() {
const LangOptions &LO = getCurrentASTUnit().getLangOpts();
if (LO.CPlusPlusModules && !LO.CurrentModule.empty()) {
-
ASTReader *R = getCurrentASTUnit().getASTReader().get();
unsigned SubModuleCount = R->getTotalNumSubmodules();
serialization::ModuleFile &MF = R->getModuleManager().getPrimaryModule();
diff --git a/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp b/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
index b280a1359d2f272..59f7f955db50971 100644
--- a/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
+++ b/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
@@ -65,6 +65,8 @@ CreateFrontendBaseAction(CompilerInstance &CI) {
return std::make_unique<GenerateModuleFromModuleMapAction>();
case GenerateModuleInterface:
return std::make_unique<GenerateModuleInterfaceAction>();
+ case GenerateThinModuleInterface:
+ return std::make_unique<GenerateThinModuleInterfaceAction>();
case GenerateHeaderUnit:
return std::make_unique<GenerateHeaderUnitAction>();
case GeneratePCH: return std::make_unique<GeneratePCHAction>();
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 0d347d86cb2eb44..ee8ba8a6874b57a 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -4593,10 +4593,12 @@ ASTWriter::ASTWriter(llvm::BitstreamWriter &Stream,
SmallVectorImpl<char> &Buffer,
InMemoryModuleCache &ModuleCache,
ArrayRef<std::shared_ptr<ModuleFileExtension>> Extensions,
- bool IncludeTimestamps, bool BuildingImplicitModule)
+ bool IncludeTimestamps, bool BuildingImplicitModule,
+ bool GeneratingThinBMI)
: Stream(Stream), Buffer(Buffer), ModuleCache(ModuleCache),
IncludeTimestamps(IncludeTimestamps),
- BuildingImplicitModule(BuildingImplicitModule) {
+ BuildingImplicitModule(BuildingImplicitModule),
+ GeneratingThinBMI(GeneratingThinBMI) {
for (const auto &Ext : Extensions) {
if (auto Writer = Ext->createExtensionWriter(*this))
ModuleFileExtensionWriters.push_back(std::move(Writer));
@@ -5403,18 +5405,20 @@ void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) {
// Add a trailing update record, if any. These must go last because we
// lazily load their attached statement.
- if (HasUpdatedBody) {
- const auto *Def = cast<FunctionDecl>(D);
- Record.push_back(UPD_CXX_ADDED_FUNCTION_DEFINITION);
- Record.push_back(Def->isInlined());
- Record.AddSourceLocation(Def->getInnerLocStart());
- Record.AddFunctionDefinition(Def);
- } else if (HasAddedVarDefinition) {
- const auto *VD = cast<VarDecl>(D);
- Record.push_back(UPD_CXX_ADDED_VAR_DEFINITION);
- Record.push_back(VD->isInline());
- Record.push_back(VD->isInlineSpecified());
- Record.AddVarDeclInit(VD);
+ if (!GeneratingThinBMI || MayDefAffectABI(D)) {
+ if (HasUpdatedBody) {
+ const auto *Def = cast<FunctionDecl>(D);
+ Record.push_back(UPD_CXX_ADDED_FUNCTION_DEFINITION);
+ Record.push_back(Def->isInlined());
+ Record.AddSourceLocation(Def->getInnerLocStart());
+ Record.AddFunctionDefinition(Def);
+ } else if (HasAddedVarDefinition) {
+ const auto *VD = cast<VarDecl>(D);
+ Record.push_back(UPD_CXX_ADDED_VAR_DEFINITION);
+ Record.push_back(VD->isInline());
+ Record.push_back(VD->isInlineSpecified());
+ Record.AddVarDeclInit(VD);
+ }
}
OffsetsRecord.push_back(GetDeclRef(D));
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index 763b610288f1182..00a04ee2fa730d1 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -16,6 +16,7 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/DeclVisitor.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/ODRHash.h"
#include "clang/AST/OpenMPClause.h"
#include "clang/AST/PrettyDeclStackTrace.h"
#include "clang/Basic/SourceManager.h"
@@ -40,11 +41,14 @@ namespace clang {
serialization::DeclCode Code;
unsigned AbbrevToUse;
+ bool GeneratingThinBMI = false;
+
public:
ASTDeclWriter(ASTWriter &Writer, ASTContext &Context,
- ASTWriter::RecordDataImpl &Record)
+ ASTWriter::RecordDataImpl &Record, bool GeneratingThinBMI)
: Writer(Writer), Context(Context), Record(Writer, Record),
- Code((serialization::DeclCode)0), AbbrevToUse(0) {}
+ Code((serialization::DeclCode)0), AbbrevToUse(0),
+ GeneratingThinBMI(GeneratingThinBMI) {}
uint64_t Emit(Decl *D) {
if (!Code)
@@ -270,6 +274,35 @@ namespace clang {
};
}
+bool clang::MayDefAffectABI(const Decl *D) {
+ if (auto *FD = dyn_cast<FunctionDecl>(D)) {
+ if (FD->isInlined() || FD->isConstexpr())
+ return true;
+
+ // Non-user-provided functions get emitted as weak definitions with every
+ // use, no matter whether they've been explicitly instantiated etc.
+ if (!FD->isUserProvided())
+ return true;
+
+ if (FD->isDependentContext())
+ return true;
+
+ if (FD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation)
+ return true;
+ }
+
+ if (auto *VD = dyn_cast<VarDecl>(D)) {
+ if (!VD->getDeclContext()->getRedeclContext()->isFileContext() ||
+ VD->isInline() || VD->isConstexpr() || isa<ParmVarDecl>(VD))
+ return true;
+
+ if (VD->getTemplateSpecializationKind() == TSK_ImplicitInstantiation)
+ return true;
+ }
+
+ return false;
+}
+
void ASTDeclWriter::Visit(Decl *D) {
DeclVisitor<ASTDeclWriter>::Visit(D);
@@ -285,9 +318,12 @@ void ASTDeclWriter::Visit(Decl *D) {
// have been written. We want it last because we will not read it back when
// retrieving it from the AST, we'll just lazily set the offset.
if (auto *FD = dyn_cast<FunctionDecl>(D)) {
- Record.push_...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
It was suggested to avoid "thin" and "fat" terminology (I started using it with LTO as a precedent for the terms), but I suppose something more descriptive could be selected. |
Close llvm#71034 This patch introduces thin BMI, which doesn't contain the definitions of functions and variables if its definitions won't contribute to the ABI. Testing is a big part of the patch. We want to make sure the thin BMI contains the same behavior with the existing and relatively stable fatBMI. This is pretty helpful for further reduction. For user interfeaces, this patch introduces `-fthinBMI-output=` arguments to specify the position of thin BMI. This should be used when compiling a single module unit. The design is helpful to use thin BMI in two phase compilations too. With thin BMI, In two phase compilations, we'll generate 2 BMIs, one thin BMI for being used by consumers, one fat BMI for compiling itself to object files. Maybe it sounds confusing to have 2 BMIs for one module unit. But only the thin BMI will be the BMI we're talking about generally and the fat BMI is only visible by the module unit itself. With one phase compilation, we may find the behavior of `-fthinBMI-output=` is pretty similar with `-fmodule-output=`, except one generating thin BMI and the other generating fat BMI. The design here is based on 2 things: (1) The serialization of C++ is pretty complex. We can't be sure we're handling every detail correctly in the every beginning. (2) The fat BMI is relatively widely used and relatively stable. So it looks not good to replace the fat BMI immediately with thin BMI. But, of course, in the end of the day, we want the consumers to use the thin BMI only. When that day comes, the `-fmodule-output=` will be an alias to `-fthinBMI-output=`. Another design choice is to reuse `-fmodule-output=` and introduce a flag `-femit-thin-BMI`. Then `-femit-thin-BMI -fmodule-output=` will have the same effect with `-fthinBMI-output=` now. The flag `-femit-thin-BMI` should be opt-in now and opt-off later and finally deprecated. The roadmap for thin BMI in my mind is: (1) In clang18, release thin BMI and mark it as experimental. Also encourage users and build systems to try this new mode. (2) In clang19 or clang20 (based on the issue feedbacks), remove the experimental mark for thin BMI and mark fat BMI as deprecated to be used by consumers. (3) In clang21 or clang22, error out if we found the users are trying to import a fat BMI.
|
I'll also note that my prior usage was in reference to embedding transitive BMI references in the BMI, not about including function definitions. |
Any suggestions? My english is not good, you know.
May you elaborate on this? I don't understand since I feel they are unrelated. Are you saying: #62707? I feel they are orthogonal. |
tbaederr
reviewed
Nov 8, 2023
| @@ -270,6 +274,35 @@ namespace clang { | |||
| }; | |||
| } | |||
|
|
|||
| bool clang::MayDefAffectABI(const Decl *D) { | |||
| if (auto *FD = dyn_cast<FunctionDecl>(D)) { | |||
There was a problem hiding this comment.
Suggested change
| if (auto *FD = dyn_cast<FunctionDecl>(D)) { | |
| if (const auto *FD = dyn_cast<FunctionDecl>(D)) { |
| return true; | ||
| } | ||
|
|
||
| if (auto *VD = dyn_cast<VarDecl>(D)) { |
There was a problem hiding this comment.
Suggested change
| if (auto *VD = dyn_cast<VarDecl>(D)) { | |
| if (const auto *VD = dyn_cast<VarDecl>(D)) { |
|
[edit] perhaps, in this context, we can use "sparse" instead of "thin" - but I do not have a good short word replace 'fat'. |
|
It could also be a |
|
The term |
|
Let's discuss the higher level problems in https://discourse.llvm.org/t/rfc-c-20-modules-introduce-thin-bmi-and-decls-hash/74755 |
|
The words probably don't need to be short. Interface BMI Seem like fine, accurate terms. I guess we could say "BMInterface" and "BMImplementation" but probably best to jus tgloss over "I" being "interface" and have "interface BMI" and "implementation BMI" |
|
Since there is no meaningful review opinions here, let's continue it in a new and clean PR: #75894 |
Note that this leaves behind all who have hit "Subscribe" on this PR to keep tabs on things (I wish Github provided a way to mark-as-duplicate and merge the subscriber list). All of the references to this PR are now confused as well. What benefit does a "clean" PR provide? |
From my experience, a cleaner PR will be easier for new people to get the context or we look back after times. The more the comments are, the more confused readers get. Generally a cleaner PR is not suggested since it lost the context. But for this specific example, I think it makes sense since the context here is not so meaning full.
Sorry for that. But I think it may not be a big deal since people who still want to take an eye this can made it by clicking two times simply. |
The timing is also unfortunate as some may have already gone on vacation and will miss updates until they return. Anyways, what's done is done.
It is unfortunate that Github sucks so much at rendering discussions (e.g., GItLab's threads that can be resolved makes it easy to collapse down finished things), There is the "mark as off-topic" strategy that I've seen used before, but that is also conflating the desire to hide things with spam control. In the future I would suggest mentioning those who have participated in the prior PR directly so that there's no gap in email/notification coverage. |
|
Yeah, it'll be much better if we had that feature. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close #71034
This patch introduces thin BMI, which doesn't contain the definitions of functions and variables if its definitions won't contribute to the ABI.
Testing is a big part of the patch. We want to make sure the thin BMI contains the same behavior with the existing and relatively stable fatBMI. This is pretty helpful for further reduction.
For user interfeaces, this patch introduces
-fthinBMI-output=arguments to specify the position of thin BMI. This should be used when compiling a single module unit.The design is helpful to use thin BMI in two phase compilations too. With thin BMI, In two phase compilations, we'll generate 2 BMIs, one thin BMI for being used by consumers, one fat BMI for compiling itself to object files. Maybe it sounds confusing to have 2 BMIs for one module unit. But only the thin BMI will be the BMI we're talking about generally and the fat BMI is only visible by the module unit itself.
With one phase compilation, we may find the behavior of
-fthinBMI-output=is pretty similar with-fmodule-output=, except one generating thin BMI and the other generating fat BMI. The design here is based on 2 things:(1) The serialization of C++ is pretty complex. We can't be sure we're handling every detail correctly in the every beginning.
(2) The fat BMI is relatively widely used and relatively stable. So it looks not good to replace the fat BMI immediately with thin BMI.
But, of course, in the end of the day, we want the consumers to use the thin BMI only. When that day comes, the
-fmodule-output=will be an alias to-fthinBMI-output=.Another design choice is to reuse
-fmodule-output=and introduce a flag-femit-thin-BMI. Then-femit-thin-BMI -fmodule-output=will have the same effect with-fthinBMI-output=now.The flag
-femit-thin-BMIshould be opt-in now and opt-off later and finally deprecated.The roadmap for thin BMI in my mind is:
(1) In clang18, release thin BMI and mark it as experimental. Also encourage users and build systems to try this new mode. (2) In clang19 or clang20 (based on the issue feedbacks), remove the experimental mark for thin BMI and mark fat BMI as deprecated to be used by consumers.
(3) In clang21 or clang22, error out if we found the users are trying to import a fat BMI.
CC: @mathstuf