[C++20] [Modules] Introduce -fexperimental-modules-reduced-bmi #85050
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Chuanqi Xu (ChuanqiXu9) ChangesThis is the driver part of #75894. This patch introduces '-fgen-reduced-bmi' to enable generating the reduced BMI. This patch did:
The core design idea is that users should be able to enable this easily with the existing cmake mechanisms. The future plan for the flag is:
I'll send release notes and document in seperate commits after this get landed. Patch is 20.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/85050.diff 13 Files Affected:
diff --git a/clang/include/clang/CodeGen/CodeGenAction.h b/clang/include/clang/CodeGen/CodeGenAction.h
index 7ad2988e589eb2..186dbb43f01ef7 100644
--- a/clang/include/clang/CodeGen/CodeGenAction.h
+++ b/clang/include/clang/CodeGen/CodeGenAction.h
@@ -57,6 +57,8 @@ class CodeGenAction : public ASTFrontendAction {
bool loadLinkModules(CompilerInstance &CI);
protected:
+ bool BeginSourceFileAction(CompilerInstance &CI) override;
+
/// Create a new code generation action. If the optional \p _VMContext
/// parameter is supplied, the action uses it without taking ownership,
/// otherwise it creates a fresh LLVM context and takes ownership.
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index aca8c9b0d5487a..b8ceeff9362df9 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3018,6 +3018,7 @@ defm prebuilt_implicit_modules : BoolFOption<"prebuilt-implicit-modules",
def fmodule_output_EQ : Joined<["-"], "fmodule-output=">,
Flags<[NoXarchOption]>, Visibility<[ClangOption, CC1Option]>,
+ MarshallingInfoString<FrontendOpts<"ModuleOutputPath">>,
HelpText<"Save intermediate module file results when compiling a standard C++ module unit.">;
def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption]>,
Visibility<[ClangOption, CC1Option]>,
@@ -3031,6 +3032,11 @@ defm skip_odr_check_in_gmf : BoolOption<"f", "skip-odr-check-in-gmf",
"Perform ODR checks for decls in the global module fragment.">>,
Group<f_Group>;
+def gen_reduced_bmi : Flag<["-"], "fgen-reduced-bmi">,
+ Group<i_Group>, Visibility<[ClangOption, CC1Option]>,
+ HelpText<"Generate the reduced BMI">,
+ MarshallingInfoFlag<FrontendOpts<"GenReducedBMI">>;
+
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">,
diff --git a/clang/include/clang/Frontend/FrontendOptions.h b/clang/include/clang/Frontend/FrontendOptions.h
index 8085dbcbf671a6..ddfd4f30d1b773 100644
--- a/clang/include/clang/Frontend/FrontendOptions.h
+++ b/clang/include/clang/Frontend/FrontendOptions.h
@@ -387,6 +387,10 @@ class FrontendOptions {
LLVM_PREFERRED_TYPE(bool)
unsigned ModulesShareFileManager : 1;
+ /// Whether to generate reduced BMI for C++20 named modules.
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned GenReducedBMI : 1;
+
CodeCompleteOptions CodeCompleteOpts;
/// Specifies the output format of the AST.
@@ -553,6 +557,9 @@ class FrontendOptions {
/// Path which stores the output files for -ftime-trace
std::string TimeTracePath;
+ /// Output Path for module output file.
+ std::string ModuleOutputPath;
+
public:
FrontendOptions()
: DisableFree(false), RelocatablePCH(false), ShowHelp(false),
@@ -565,7 +572,7 @@ class FrontendOptions {
BuildingImplicitModuleUsesLock(true), ModulesEmbedAllFiles(false),
IncludeTimestamps(true), UseTemporary(true),
AllowPCMWithCompilerErrors(false), ModulesShareFileManager(true),
- TimeTraceGranularity(500) {}
+ GenReducedBMI(false), TimeTraceGranularity(500) {}
/// getInputKindForExtension - Return the appropriate input kind for a file
/// extension. For example, "c" would return Language::C.
diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h
index 3ed9803fa3745b..bd310b6c7a5cdd 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -846,7 +846,7 @@ class ASTWriter : public ASTDeserializationListener,
/// AST and semantic-analysis consumer that generates a
/// precompiled header from the parsed source code.
class PCHGenerator : public SemaConsumer {
- const Preprocessor &PP;
+ Preprocessor &PP;
std::string OutputFile;
std::string isysroot;
Sema *SemaPtr;
@@ -867,11 +867,12 @@ class PCHGenerator : public SemaConsumer {
DiagnosticsEngine &getDiagnostics() const {
return SemaPtr->getDiagnostics();
}
+ Preprocessor &getPreprocessor() { return PP; }
virtual Module *getEmittingModule(ASTContext &Ctx);
public:
- PCHGenerator(const Preprocessor &PP, InMemoryModuleCache &ModuleCache,
+ PCHGenerator(Preprocessor &PP, InMemoryModuleCache &ModuleCache,
StringRef OutputFile, StringRef isysroot,
std::shared_ptr<PCHBuffer> Buffer,
ArrayRef<std::shared_ptr<ModuleFileExtension>> Extensions,
@@ -893,7 +894,7 @@ class ReducedBMIGenerator : public PCHGenerator {
virtual Module *getEmittingModule(ASTContext &Ctx) override;
public:
- ReducedBMIGenerator(const Preprocessor &PP, InMemoryModuleCache &ModuleCache,
+ ReducedBMIGenerator(Preprocessor &PP, InMemoryModuleCache &ModuleCache,
StringRef OutputFile);
void HandleTranslationUnit(ASTContext &Ctx) override;
diff --git a/clang/lib/CodeGen/CodeGenAction.cpp b/clang/lib/CodeGen/CodeGenAction.cpp
index bb9aaba025fa59..3737c99847f04f 100644
--- a/clang/lib/CodeGen/CodeGenAction.cpp
+++ b/clang/lib/CodeGen/CodeGenAction.cpp
@@ -25,8 +25,11 @@
#include "clang/CodeGen/ModuleBuilder.h"
#include "clang/Driver/DriverDiagnostic.h"
#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
#include "clang/Frontend/FrontendDiagnostic.h"
+#include "clang/Frontend/MultiplexConsumer.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Serialization/ASTWriter.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/Bitcode/BitcodeReader.h"
#include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
@@ -1003,6 +1006,12 @@ CodeGenerator *CodeGenAction::getCodeGenerator() const {
return BEConsumer->getCodeGenerator();
}
+bool CodeGenAction::BeginSourceFileAction(CompilerInstance &CI) {
+ if (CI.getFrontendOpts().GenReducedBMI)
+ CI.getLangOpts().setCompilingModule(LangOptions::CMK_ModuleInterface);
+ return true;
+}
+
static std::unique_ptr<raw_pwrite_stream>
GetOutputStream(CompilerInstance &CI, StringRef InFile, BackendAction Action) {
switch (Action) {
@@ -1061,6 +1070,16 @@ CodeGenAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) {
CI.getPreprocessor().addPPCallbacks(std::move(Callbacks));
}
+ if (CI.getFrontendOpts().GenReducedBMI &&
+ !CI.getFrontendOpts().ModuleOutputPath.empty()) {
+ std::vector<std::unique_ptr<ASTConsumer>> Consumers{2};
+ Consumers[0] = std::move(Result);
+ Consumers[1] = std::make_unique<ReducedBMIGenerator>(
+ CI.getPreprocessor(), CI.getModuleCache(),
+ CI.getFrontendOpts().ModuleOutputPath);
+ return std::make_unique<MultiplexConsumer>(std::move(Consumers));
+ }
+
return std::move(Result);
}
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 190782a79a2456..872ea905f01a03 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -4737,6 +4737,14 @@ Action *Driver::ConstructPhaseAction(
if (Args.hasArg(options::OPT_extract_api))
return C.MakeAction<ExtractAPIJobAction>(Input, types::TY_API_INFO);
+ // With '-fgen-reduced-bmi', we don't want to run the precompile phase
+ // unless the user specified '--precompile'. In the case the '--precompile'
+ // flag is enabled, we will try to emit the reduced BMI as a by product
+ // in GenerateModuleInterfaceAction.
+ if (Args.hasArg(options::OPT_gen_reduced_bmi) &&
+ !Args.getLastArg(options::OPT__precompile))
+ return Input;
+
types::ID OutputTy = getPrecompiledType(Input->getType());
assert(OutputTy != types::TY_INVALID &&
"Cannot precompile this input type!");
@@ -5802,19 +5810,8 @@ static const char *GetModuleOutputPath(Compilation &C, const JobAction &JA,
(C.getArgs().hasArg(options::OPT_fmodule_output) ||
C.getArgs().hasArg(options::OPT_fmodule_output_EQ)));
- if (Arg *ModuleOutputEQ =
- C.getArgs().getLastArg(options::OPT_fmodule_output_EQ))
- return C.addResultFile(ModuleOutputEQ->getValue(), &JA);
-
- SmallString<64> OutputPath;
- Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o);
- if (FinalOutput && C.getArgs().hasArg(options::OPT_c))
- OutputPath = FinalOutput->getValue();
- else
- OutputPath = BaseInput;
-
- const char *Extension = types::getTypeTempSuffix(JA.getType());
- llvm::sys::path::replace_extension(OutputPath, Extension);
+ SmallString<256> OutputPath =
+ tools::getCXX20NamedModuleOutputPath(C.getArgs(), BaseInput);
return C.addResultFile(C.getArgs().MakeArgString(OutputPath.c_str()), &JA);
}
@@ -5901,8 +5898,10 @@ const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
// If we're emitting a module output with the specified option
// `-fmodule-output`.
if (!AtTopLevel && isa<PrecompileJobAction>(JA) &&
- JA.getType() == types::TY_ModuleFile && SpecifiedModuleOutput)
+ JA.getType() == types::TY_ModuleFile && SpecifiedModuleOutput) {
+ assert(!C.getArgs().hasArg(options::OPT_gen_reduced_bmi));
return GetModuleOutputPath(C, JA, BaseInput);
+ }
// Output to a temporary file?
if ((!AtTopLevel && !isSaveTempsEnabled() &&
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 3a7a1cf99c79ac..7877c1d873bb04 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3777,6 +3777,24 @@ bool Driver::getDefaultModuleCachePath(SmallVectorImpl<char> &Result) {
return false;
}
+llvm::SmallString<256>
+clang::driver::tools::getCXX20NamedModuleOutputPath(const ArgList &Args,
+ const char *BaseInput) {
+ if (Arg *ModuleOutputEQ = Args.getLastArg(options::OPT_fmodule_output_EQ))
+ return StringRef(ModuleOutputEQ->getValue());
+
+ SmallString<256> OutputPath;
+ if (Arg *FinalOutput = Args.getLastArg(options::OPT_o);
+ FinalOutput && Args.hasArg(options::OPT_c))
+ OutputPath = FinalOutput->getValue();
+ else
+ OutputPath = BaseInput;
+
+ const char *Extension = types::getTypeTempSuffix(types::TY_ModuleFile);
+ llvm::sys::path::replace_extension(OutputPath, Extension);
+ return OutputPath;
+}
+
static bool RenderModulesOptions(Compilation &C, const Driver &D,
const ArgList &Args, const InputInfo &Input,
const InputInfo &Output, bool HaveStd20,
@@ -3965,9 +3983,26 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
// module fragment.
CmdArgs.push_back("-fskip-odr-check-in-gmf");
- // Claim `-fmodule-output` and `-fmodule-output=` to avoid unused warnings.
- Args.ClaimAllArgs(options::OPT_fmodule_output);
- Args.ClaimAllArgs(options::OPT_fmodule_output_EQ);
+ // Noop if we see '-fgen-reduced-bmi' with other translation units than module
+ // units. This is more user friendly to allow end uers to enable this feature
+ // without asking for help from build systems.
+ if (Args.hasArg(options::OPT_gen_reduced_bmi) &&
+ (Input.getType() == driver::types::TY_CXXModule ||
+ Input.getType() == driver::types::TY_PP_CXXModule)) {
+ CmdArgs.push_back("-fgen-reduced-bmi");
+
+ if (Args.hasArg(options::OPT_fmodule_output_EQ))
+ Args.AddLastArg(CmdArgs, options::OPT_fmodule_output_EQ);
+ else
+ CmdArgs.push_back(Args.MakeArgString(
+ "-fmodule-output=" +
+ getCXX20NamedModuleOutputPath(Args, Input.getBaseInput())));
+ } else {
+ // To avoid unused warnings.
+ Args.ClaimAllArgs(options::OPT_gen_reduced_bmi);
+ Args.ClaimAllArgs(options::OPT_fmodule_output);
+ Args.ClaimAllArgs(options::OPT_fmodule_output_EQ);
+ }
return HaveModules;
}
diff --git a/clang/lib/Driver/ToolChains/Clang.h b/clang/lib/Driver/ToolChains/Clang.h
index 0f503c4bd1c4fe..3b2639b4cc49f4 100644
--- a/clang/lib/Driver/ToolChains/Clang.h
+++ b/clang/lib/Driver/ToolChains/Clang.h
@@ -193,6 +193,20 @@ DwarfFissionKind getDebugFissionKind(const Driver &D,
const llvm::opt::ArgList &Args,
llvm::opt::Arg *&Arg);
+// Calculate the output path of the module file when compiling a module unit
+// with the `-fmodule-output` option or `-fmodule-output=` option specified.
+// The behavior is:
+// - If `-fmodule-output=` is specfied, then the module file is
+// writing to the value.
+// - Otherwise if the output object file of the module unit is specified, the
+// output path
+// of the module file should be the same with the output object file except
+// the corresponding suffix. This requires both `-o` and `-c` are specified.
+// - Otherwise, the output path of the module file will be the same with the
+// input with the corresponding suffix.
+llvm::SmallString<256>
+getCXX20NamedModuleOutputPath(const llvm::opt::ArgList &Args,
+ const char *BaseInput);
} // end namespace tools
} // end namespace driver
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index 81fcd8d5ae9bd3..7f1052c165d124 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -281,6 +281,13 @@ GenerateModuleInterfaceAction::CreateASTConsumer(CompilerInstance &CI,
if (Consumers.empty())
return nullptr;
+ if (CI.getFrontendOpts().GenReducedBMI &&
+ !CI.getFrontendOpts().ModuleOutputPath.empty()) {
+ Consumers.push_back(std::make_unique<ReducedBMIGenerator>(
+ CI.getPreprocessor(), CI.getModuleCache(),
+ CI.getFrontendOpts().ModuleOutputPath));
+ }
+
return std::make_unique<MultiplexConsumer>(std::move(Consumers));
}
diff --git a/clang/lib/Frontend/PrecompiledPreamble.cpp b/clang/lib/Frontend/PrecompiledPreamble.cpp
index 9b0ef30a14121b..fdf05c3613c956 100644
--- a/clang/lib/Frontend/PrecompiledPreamble.cpp
+++ b/clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -290,8 +290,7 @@ class PrecompilePreambleAction : public ASTFrontendAction {
class PrecompilePreambleConsumer : public PCHGenerator {
public:
- PrecompilePreambleConsumer(PrecompilePreambleAction &Action,
- const Preprocessor &PP,
+ PrecompilePreambleConsumer(PrecompilePreambleAction &Action, Preprocessor &PP,
InMemoryModuleCache &ModuleCache,
StringRef isysroot,
std::shared_ptr<PCHBuffer> Buffer)
diff --git a/clang/lib/Serialization/GeneratePCH.cpp b/clang/lib/Serialization/GeneratePCH.cpp
index f54db36d4a0199..4260ba867ad28b 100644
--- a/clang/lib/Serialization/GeneratePCH.cpp
+++ b/clang/lib/Serialization/GeneratePCH.cpp
@@ -14,6 +14,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/Frontend/FrontendDiagnostic.h"
#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Sema/SemaConsumer.h"
#include "clang/Serialization/ASTReader.h"
@@ -23,8 +24,8 @@
using namespace clang;
PCHGenerator::PCHGenerator(
- const Preprocessor &PP, InMemoryModuleCache &ModuleCache,
- StringRef OutputFile, StringRef isysroot, std::shared_ptr<PCHBuffer> Buffer,
+ Preprocessor &PP, InMemoryModuleCache &ModuleCache, StringRef OutputFile,
+ StringRef isysroot, std::shared_ptr<PCHBuffer> Buffer,
ArrayRef<std::shared_ptr<ModuleFileExtension>> Extensions,
bool AllowASTWithErrors, bool IncludeTimestamps,
bool BuildingImplicitModule, bool ShouldCacheASTInMemory,
@@ -88,7 +89,7 @@ ASTDeserializationListener *PCHGenerator::GetASTDeserializationListener() {
return &Writer;
}
-ReducedBMIGenerator::ReducedBMIGenerator(const Preprocessor &PP,
+ReducedBMIGenerator::ReducedBMIGenerator(Preprocessor &PP,
InMemoryModuleCache &ModuleCache,
StringRef OutputFile)
: PCHGenerator(
@@ -101,12 +102,26 @@ ReducedBMIGenerator::ReducedBMIGenerator(const Preprocessor &PP,
Module *ReducedBMIGenerator::getEmittingModule(ASTContext &Ctx) {
Module *M = Ctx.getCurrentNamedModule();
- assert(M->isNamedModuleUnit() &&
+ assert(M && M->isNamedModuleUnit() &&
"ReducedBMIGenerator should only be used with C++20 Named modules.");
return M;
}
void ReducedBMIGenerator::HandleTranslationUnit(ASTContext &Ctx) {
+ // FIMXE: We'd better to wrap such options to a new class ASTWriterOptions.
+ getPreprocessor()
+ .getHeaderSearchInfo()
+ .getHeaderSearchOpts()
+ .ModulesSkipDiagnosticOptions = true;
+ getPreprocessor()
+ .getHeaderSearchInfo()
+ .getHeaderSearchOpts()
+ .ModulesSkipHeaderSearchPaths = true;
+ getPreprocessor()
+ .getHeaderSearchInfo()
+ .getHeaderSearchOpts()
+ .ModulesSkipPragmaDiagnosticMappings = true;
+
PCHGenerator::HandleTranslationUnit(Ctx);
if (!isComplete())
diff --git a/clang/test/Driver/module-fgen-reduced-bmi.cppm b/clang/test/Driver/module-fgen-reduced-bmi.cppm
new file mode 100644
index 00000000000000..77afdf3e74e505
--- /dev/null
+++ b/clang/test/Driver/module-fgen-reduced-bmi.cppm
@@ -0,0 +1,53 @@
+// It is annoying to handle different slash direction
+// in Windows and Linux. So we disable the test on Windows
+// here.
+// REQUIRES: !system-windows
+// On AIX, the default output for `-c` may be `.s` instead of `.o`,
+// which makes the test fail. So disable the test on AIX.
+// REQUIRES: !system-aix
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output=%t/Hello.pcm \
+// RUN: -fgen-reduced-bmi -c -o %t/Hello.o -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm \
+// RUN: -fgen-reduced-bmi -c -o %t/Hello.o -### 2>&1 | \
+// RUN: FileCheck %t/Hello.cppm --check-prefix=CHECK-UNSPECIFIED
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm \
+// RUN: -fgen-reduced-bmi -c -### 2>&1 | \
+// RUN: FileCheck %t/Hello.cppm --check-prefix=CHECK-NO-O
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm \
+// RUN: -fgen-reduced-bmi -c -o %t/AnotherName.o -### 2>&1 | \
+// RUN: FileCheck %t/Hello.cppm --check-prefix=CHECK-ANOTHER-NAME
+//
+// RUN: %clang -std=c++20 %t/Hello.cppm --precompile -fgen-reduced-bmi \
+// RUN: -o %t/Hello.full.pcm -### 2>&1 | FileCheck %t/Hello.cppm \
+// RUN: --check-prefix=CHECK-EMIT-MODULE-INTERFACE
+//
+// RUN: %clang -std=c++20 %t/Hello.cc -fgen-reduced-bmi -Wall -Werror \
+// RUN: -c -o %t/Hello.o -### 2>&1 | FileCheck %t/Hello.cc
+
+//--- Hello.cppm
+export module Hello;
+
+// Test that we won't generate the emit-module-interface as 2 phase compilation model.
+// CHECK-NOT: -emit-module-interface
+// CHECK: "-fgen-reduced-bmi"
+
+// CHECK-UNSPECIFIED: -fmodule-output={{.*}}/Hello.pcm
+
+// CHECK-NO-O: -fmodule-output={{.*}}/Hello.pcm
+// CHECK-ANOTHER-NAME: -fmodule-output={{.*}}/AnotherName.pcm
+
+// With `-emit-module-interface` specified, we should still see the `-emit-module-interface`
+// flag.
+// CHECK-EMIT-MODULE-INTERFACE: -emit-module-interface
+
+//--- Hello.cc
+
+// CHECK-NOT: "-fgen-reduced-bmi"
diff --git a/clang/test/Modules/gen-reduced-bmi.cppm b/clang/test/Modules/gen-reduced-bmi.cppm
new file mode 100644
index 00000000000000..8c08795ce4e453
--- /dev/null
+++ b/clang/test/Modules/gen-reduced-bmi.cppm
@@ -0,0 +1,36 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -emit-reduced-module-interface -o %t/a.reduced.pcm
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -fgen-reduced-bmi -fmodule-output=%t/a.pcm \
+// RUN: -S -emit-llvm -o %t/a.ll
+//
+// Test that the generated BMI from `-fgen-reduced-bmi -fmodule-output=` is same with
+// `-emit-reduced-module-interface`.
+// RUN: diff %t/a.reduced.pcm %t/a.pcm
+//
+// Test that we can consume the produced BMI correctly.
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -fmodule-file=a=%t/a.pcm -fsyntax-only -verify
+//
+// RUN: rm -f %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 %t/a.cppm -fgen-reduced-bmi -fmodule-output=%t/a.pcm \
+// RUN: -emit-module-interface -o %t/a.full.pcm
+// RUN: diff %t/a.reduced.pcm %t/a.pcm
+// RUN: not diff %t/a.pcm %t/a.full.pcm
+//
+// RUN: %clang_cc1 -std=c++20 %t/b.cppm -...
[truncated]
|
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
from
77c6642
to
e5114cd
Compare
|
Using // a.hpp
template <typename T>
void ignore(T const &) noexcept {}
inline void resultCheck(char const *message) {
ignore(message);
}
// b.cppm
module;
#include "a.hpp"
export module b;
export {
using ignore;
using resultCheck;
}
// c.cppm
export module c;
import b;
export void test() {
resultCheck(nullptr);
}This will result in this linker error: |
|
Do I miss something? The performance and file size is similar with and without |
There was a problem hiding this comment.
I have no issue with the general intention or phasing here; my main concern is that we are introducing [yet] another user-facing modules option that actually we intend will become irrelevant after a few releases.
In practice, removing user-facing options is not very easy.
So:
- Perhaps we could have only the cc1 option and you could introduce some temporary handling in the driver to recognise it and amend the --precompile behaviour?
- Maybe the project could introduce something like -fexperimental-xxxx-xxxx with a clear statement that fexperimental flags cannot be relied on to be stable (or even present) in any future release)?
| @@ -3031,6 +3032,11 @@ defm skip_odr_check_in_gmf : BoolOption<"f", "skip-odr-check-in-gmf", | |||
| "Perform ODR checks for decls in the global module fragment.">>, | |||
| Group<f_Group>; | |||
|
|
|||
| def gen_reduced_bmi : Flag<["-"], "fgen-reduced-bmi">, | |||
There was a problem hiding this comment.
If this is going to be a user-facing flag (even for a few releases) then I think it should be spelled in a way that make it modules-relatated (e.g. -fmodules-reduce-bmi, or -fmodules-minimise-bmi), as you know I'm not a fan of increasing the already huge number of modules-related user-facing options... (but I'll comment on that separately)
There was a problem hiding this comment.
Done by using -fmodules-reduce-bmi
|
+1 to @iains's comments about being careful about the introduction and naming of driver flags & probably avoid it in this case, if possible, or try to make it clearly experimental. |
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
from
e5114cd
to
527b69e
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@iains @dwblaikie Understood. And I thought the major problem is that there are a lot flags from clang modules. And it is indeed confusing. But given we have to introduce new flags in this case, probably we can only try to make it more clear by better documents. |
This may be fixed in trunk. This should be a bug in Reduced BMI. Note that the current patch is only the driver part of Reduced BMI. And this is the reason why we need a new flag. Otherwise I'll propose to enable it by default : ) |
Currently, the Reduced BMI will only remove function bodies for non-inline functions and definitions for some variables... And declarations in headers or GMF are primarily inline, so probably the current Reduced BMI won't produce a strong impact on modules whose contents are primarily in GMF as expectedly. For example, I can observe some impact on https://github.com/alibaba/async_simple/tree/CXX20Modules, where the repo has more use codes in the module purview instead of the GMF. Further, in my mind, we can reduce more things in Reduced BMI. An example may be #76930. Thanks for testing : ) |
ChuanqiXu9
changed the title
So you do not think it possible to restrict the new flag to be "internal" (i.e. cc1-only) and to put some temporary driver processing to handle that? (I agree that this is an unusual mechanism, but the idea is to recognise that the driver-side processing is only expected to me temporary). |
I have no idea how can we make that. We still need the users to provide something to enable reduced BMI. And I think it is symmetric to a new flag. |
What I mean is that (a) we need the internal 'cc1' flag permanently; but (b) we do not expect to need a user-facing driver flag permanently. and (c) We want to allow users to try this out. I am suggesting we could say "to try this out use -Xclang -fmodules-reduced-bmi" and have temporary code in the driver to deal with the changes needed to phasing. If this is not possible. then I suppose I am a bit sad that we keep saying 'there are too many modules options' - but yet still add more. however - we need to make progress, so if the suggestion here is really a non-starter .. then such is life. Perhaps the second suggestion (-fexperimental-xxxxx options) could be discussed at the project level. |
Got your point. But I feel the And yes, it is indeed that we have a lot modules options. But I think this may not be the blocking point for adding new modules options if we think it is really needed. If there are things needed to be added by flags really and we don't do that due to we feel we have too many flags, I feel it is somewhat "not eating for fear of choking". I think what we need to do for this case is to be pretty careful when adding new flags and I think we already make it for Reduced BMI. |
That seems good to me - these are pretty experimental directions we're going in. We haven't figured out how this stuff should work long term - we are experimenting.
What do you mean by "symmetric"? The difference @iains is trying to highlight is that adding driver flags is a long-term commitment burden - adding cc1 flags, clarifying that these are not long term supported/guaranteed parts of the interface, but a way to experiment with some things until we figure out what the long term supported interface is. Especially if we think the long-term future is always (or, at least by default) reduced BMIs, experimenting with it under a flag until we have confidence in that direction - then maybe just changing the Clang behavior (again, perhaps with a cc1 flag to opt-out as an escape hatch that's intended to be short-term while we figure out whatever bugs lead to the need to use that escape hatch - then when we fixt he bugs and remove the flag, the people on the bleeding edge will need to remove the flag, which is fine/good - rather than leaving these weird flags around forever). |
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
from
527b69e
to
bd93f31
Compare
ChuanqiXu9
changed the title
|
Got it. I've renamed the flag as
I mean, we always want the users to do something explicitly to enable the feature. But this might not be important now. |
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
from
bd93f31
to
a0ec93e
Compare
Thanks.
I think the point here is that we want expert users to try this out (with understanding that it might not behave exactly as they expect). Because it is experimental, it is not yet ready for "end users" - I would expect expert users to be able to deal with -Xclang xxxxx (but I will not insist, it's just a suggestion) |
Agreed. |
|
I'd like to land this patch in next week if no objection comes in. |
| @@ -3017,6 +3017,7 @@ defm prebuilt_implicit_modules : BoolFOption<"prebuilt-implicit-modules", | |||
|
|
|||
| def fmodule_output_EQ : Joined<["-"], "fmodule-output=">, | |||
| Flags<[NoXarchOption]>, Visibility<[ClangOption, CC1Option]>, | |||
| MarshallingInfoString<FrontendOpts<"ModuleOutputPath">>, | |||
There was a problem hiding this comment.
What's the motivation to include this change in this patch? Could it be separated?
There was a problem hiding this comment.
Since the flag was only used in drivers. So we didn't need a variable. But after this patch, we need a variable. If we separate this out, we may get a unused variable in that patch. I feel it is not so good.
…o a seperate function Required in the review process of #85050.
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
from
a0ec93e
to
dc0856a
Compare
| "ReducedBMIGenerator should only be used with C++20 Named modules."); | ||
| return M; | ||
| } | ||
|
|
||
| void ReducedBMIGenerator::HandleTranslationUnit(ASTContext &Ctx) { | ||
| // FIMXE: We'd better to wrap such options to a new class ASTWriterOptions. | ||
| getPreprocessor() |
There was a problem hiding this comment.
What's the motivation for this change/how does it relate to the purpose of this patch?
(& it might be easier to read if the HeaderSearchOpts were accessed once, stored in a reference, then the various settings were made:
HeaderSearchOptions& Opts = getPreprocessor().getHeaderSearchInfo().getHeaderSearchOpts();
Opts.ModulesSkipDiagnosticOptions = true;
Opts.ModulesSkipHeaderSearchPaths = true;
Opts.ModulesSkipPragmaDiagnosticMappings = true;
)
There was a problem hiding this comment.
Oh, my bad. The motivation is that during my local testing, I found the size of the reduced BMI may be larger than the full BMI, which should be incorrect. And then I tried to fix this on the fly... I've splitted this into a NFC patch ed1cfff
Thanks.
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
from
dc0856a
to
7f24874
Compare
…rtion This was part of #85050. It is suggested to split the unrelated change as much as possible. So here is the patch.
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
2 times, most recently
from
8baafcd
to
8c51aa5
Compare
|
Rebased with main. @dwblaikie may you have more comments? |
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
from
8c51aa5
to
7b0b9b5
Compare
clang/lib/CodeGen/CodeGenAction.cpp
Outdated
| if (CI.getFrontendOpts().GenReducedBMI && | ||
| !CI.getFrontendOpts().ModuleOutputPath.empty()) { | ||
| std::vector<std::unique_ptr<ASTConsumer>> Consumers{2}; | ||
| Consumers[0] = std::make_unique<ReducedBMIGenerator>( | ||
| CI.getPreprocessor(), CI.getModuleCache(), | ||
| CI.getFrontendOpts().ModuleOutputPath); | ||
| Consumers[1] = std::move(Result); | ||
| return std::make_unique<MultiplexConsumer>(std::move(Consumers)); | ||
| } |
There was a problem hiding this comment.
Why is this new code, rather than an alternative codepath of existing code? (like some code that was producing the BMI would go from producing the unreduced BMI to producing the reduced BMI)
Figuring this out would help me unedrstand why ModuleOutputPath is new (this patch doesn't add the functionality to output a module to a path - we had that already, this just changes the content that goes there)
I guess because maybe this patch is changing the way we output the BMI - or that the reduced BMI is being output in a different way than the non-reduced BMI?
Should we address that? Make the current full BMI emission work the same way as the reduced BMI? (or, more precisely, in a pre-patch, change the full BMI emission to use a mechanism that can then be reused for the reduced BMI?)
There was a problem hiding this comment.
For full BMI, the workflow is:
.cpp -> .pcm -> .o
no matter if -fmodule-output is specified or not. If -fmodule-output is not specified, the .pcm file will be generated in /tmp directory.
And for reduced BMI, the workflow is:
.cpp ---------> .o
-----> .pcm
that said, we generate the .o directly from the .cpp then we don't need the .pcm to contain the full information to generate the .o .
Then for the original question, can we make the full BMI in the same way with the reduced BMI? Maybe we can, but it looks like an overkill. Since if the .o don't need the .pcm, we don't need the .pcm to be full.
| "-fmodule-output=" + | ||
| getCXX20NamedModuleOutputPath(Args, Input.getBaseInput()))); |
There was a problem hiding this comment.
Would it be reasonable to push two separate arguments, instead of concatenating them?
There was a problem hiding this comment.
I am not sure. I tried to not concat it but the test gets failing. I didn't see why but I see almost we always concat it in such cases (appending a variable to some flag ends with =). So I feel it may not be problematic or we can fix such things together if we want.
ChuanqiXu9
force-pushed
the
introduce-fgen-reduced-bmi
branch
from
7b0b9b5
to
59f2817
Compare
|
Thanks for reviewing. |
…85050) This is the driver part of llvm#75894. This patch introduces '-fexperimental-modules-reduced-bmi' to enable generating the reduced BMI. This patch did: - When `-fexperimental-modules-reduced-bmi` is specified but `--precompile` is not specified for a module unit, we'll skip the precompile phase to avoid unnecessary two-phase compilation phases. Then if `-c` is specified, we will generate the reduced BMI in CodeGenAction as a by-product. - When `-fexperimental-modules-reduced-bmi` is specified and `--precompile` is specified, we will generate the reduced BMI in GenerateModuleInterfaceAction as a by-product. - When `-fexperimental-modules-reduced-bmi` is specified for a non-module unit. We don't do anything nor try to give a warn. This is more user friendly so that the end users can try to test and experiment with the feature without asking help from the build systems. The core design idea is that users should be able to enable this easily with the existing cmake mechanisms. The future plan for the flag is: - Add this to clang19 and make it opt-in for 1~2 releases. It depends on the testing feedback to decide how long we like to make it opt-in. - Then we can announce the existing BMI generating may be deprecated and suggesting people (end users or build systems) to enable this for 1~2 releases. - Finally we will enable this by default. When that time comes, the term `BMI` will refer to the reduced BMI today and the existing BMI will only be meaningful to build systems which loves to support two phase compilations. I'll send release notes and document in seperate commits after this get landed.
…85050) This is the driver part of llvm#75894. This patch introduces '-fexperimental-modules-reduced-bmi' to enable generating the reduced BMI. This patch did: - When `-fexperimental-modules-reduced-bmi` is specified but `--precompile` is not specified for a module unit, we'll skip the precompile phase to avoid unnecessary two-phase compilation phases. Then if `-c` is specified, we will generate the reduced BMI in CodeGenAction as a by-product. - When `-fexperimental-modules-reduced-bmi` is specified and `--precompile` is specified, we will generate the reduced BMI in GenerateModuleInterfaceAction as a by-product. - When `-fexperimental-modules-reduced-bmi` is specified for a non-module unit. We don't do anything nor try to give a warn. This is more user friendly so that the end users can try to test and experiment with the feature without asking help from the build systems. The core design idea is that users should be able to enable this easily with the existing cmake mechanisms. The future plan for the flag is: - Add this to clang19 and make it opt-in for 1~2 releases. It depends on the testing feedback to decide how long we like to make it opt-in. - Then we can announce the existing BMI generating may be deprecated and suggesting people (end users or build systems) to enable this for 1~2 releases. - Finally we will enable this by default. When that time comes, the term `BMI` will refer to the reduced BMI today and the existing BMI will only be meaningful to build systems which loves to support two phase compilations. I'll send release notes and document in seperate commits after this get landed.
This is the driver part of #75894.
This patch introduces '-fexperimental-modules-reduced-bmi' to enable generating the reduced BMI.
This patch did:
-fexperimental-modules-reduced-bmiis specified but--precompileis not specified for a module unit, we'll skip the precompile phase to avoid unnecessary two-phase compilation phases. Then if-cis specified, we will generate the reduced BMI in CodeGenAction as a by-product.-fexperimental-modules-reduced-bmiis specified and--precompileis specified, we will generate the reduced BMI in GenerateModuleInterfaceAction as a by-product.-fexperimental-modules-reduced-bmiis specified for a non-module unit. We don't do anything nor try to give a warn. This is more user friendly so that the end users can try to test and experiment with the feature without asking help from the build systems.The core design idea is that users should be able to enable this easily with the existing cmake mechanisms.
The future plan for the flag is:
BMIwill refer to the reduced BMI today and the existing BMI will only be meaningful to build systems which loves to support two phase compilations.I'll send release notes and document in seperate commits after this get landed.