[Driver] Add -f[no-]legacy-pass-manager to supersede -f[no-]experimental-new-pass-manager

[MaskRay]

Summary

The new PM is considered stable and many downstream groups have adopted it (some
have adopted it for more than two years). Add -f[no-]legacy-pass-manager to reflect the
fact that it is no longer experimental and the legacy pass manager is something we strive to retire.

In the future, when the legacy PM eventually goes away,
-fno-experimental-new-pass-manager and -flegacy-pass-manager will be removed.

This patch also changes -f[no-]legacy-pass-manager to pass -plugin-opt={new,legacy}-pass-manager to the linker (supported by both ld.lld and LLVMgold.so) when -flto/-flto=thin is specified

[MaskRay]

Improve tests

[aeubanks]

Makes sense to me, thanks!

[rsmith]

I don't like having new in the name of the flag; I think that's going to age badly (and I would argue it already has done; the "new pass manager" isn't really all that new any more).

Given that the new pass manager is considered stable and not experimental, can we change our default and instead add a flag to request the old behavior? If we're not ready for that yet, then perhaps the "experimental" label still fits.

[MaskRay]

Add -f[no-]legacy-pass-manager instead

[MaskRay]

Thanks!

@rsmith highlit the coroutine problem https://bugs.llvm.org/show_bug.cgi?id=48190 (" ... it breaks support for a C++20 language feature (coroutines)")

Perhaps this means that "EXPERIMENTAL" in ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER should not be removed until it is fixed?

[aeubanks]

In D92915#2444242, @MaskRay wrote:

Thanks!

@rsmith highlit the coroutine problem https://bugs.llvm.org/show_bug.cgi?id=48190 (" ... it breaks support for a C++20 language feature (coroutines)")

Perhaps this means that "EXPERIMENTAL" in ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER should not be removed until it is fixed?

I'm looking at this right now, it's the last issue, aside from AMDGPU passes not being ported and some code size regressions, blocking the new pass manager.

[MaskRay]

-plugin-opt=no-new-pass-manager => -plugin-opt=legacy-pass-manager

[thakis]

Seems to break tests: http://45.33.8.238/linux/34907/step_7.txt

[aeubanks]

In D92915#2444287, @thakis wrote:

Seems to break tests: http://45.33.8.238/linux/34907/step_7.txt

that test fails when the NPM is turned on and lit isn't aware of it :)

clang/include/clang/Driver/Options.td

1354–1361

this turns on the new pass manager for clang

[MaskRay]

clang/include/clang/Driver/Options.td

1354–1361

My bad. Has been fixed in 872bf680011a71b73e9bd353449d9c42ebf6b8f6

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(DiscardValueNames , 1, 0) ///< Discard Value Names from the IR (LLVMContext flag)
 CODEGENOPT(DisableLLVMPasses , 1, 0) ///< Don't run any LLVM IR passes to get
                                      ///< the pristine IR generated by the
                                      ///< frontend.
 CODEGENOPT(DisableLifetimeMarkers, 1, 0) ///< Don't emit any lifetime markers
 CODEGENOPT(DisableO0ImplyOptNone , 1, 0) ///< Don't annonate function with optnone at O0
 CODEGENOPT(ExperimentalStrictFloatingPoint, 1, 0) ///< Enables the new, experimental
                                                   ///< strict floating point.
-CODEGENOPT(ExperimentalNewPassManager, 1, 0) ///< Enables the new, experimental
-                                             ///< pass manager.
+CODEGENOPT(LegacyPassManager, 1, 0) ///< Use the legacy pass manager.
 CODEGENOPT(DebugPassManager, 1, 0) ///< Prints debug information for the new
                                    ///< pass manager.
 CODEGENOPT(DisableRedZone    , 1, 0) ///< Set when -mno-red-zone is enabled.
 CODEGENOPT(EmitCallSiteInfo, 1, 0) ///< Emit call site info only in the case of
                                    ///< '-g' + 'O>0' level.
 CODEGENOPT(IndirectTlsSegRefs, 1, 0) ///< Set when -mno-tls-direct-seg-refs
                                      ///< is specified.
 CODEGENOPT(DisableTailCalls  , 1, 0) ///< Do not emit tail calls.

clang/include/clang/Driver/Options.td

def finline_functions : Flag<["-"], "finline-functions">, Group<f_clang_Group>, Flags<[CC1Option]>,
   HelpText<"Inline suitable functions">;
 def finline_hint_functions: Flag<["-"], "finline-hint-functions">, Group<f_clang_Group>, Flags<[CC1Option]>,
   HelpText<"Inline functions which are (explicitly or implicitly) marked inline">;
 def finline : Flag<["-"], "finline">, Group<clang_ignored_f_Group>;
 def fglobal_isel : Flag<["-"], "fglobal-isel">, Group<f_clang_Group>,
   HelpText<"Enables the global instruction selector">;
 def fexperimental_isel : Flag<["-"], "fexperimental-isel">, Group<f_clang_Group>,
   Alias<fglobal_isel>;
-defm experimental_new_pass_manager : BooleanMarshalledFFlag<"experimental-new-pass-manager", "CodeGenOpts.ExperimentalNewPassManager",
-  "static_cast<unsigned>(LLVM_ENABLE_NEW_PASS_MANAGER)", "Enables an experimental new pass manager in LLVM.",
-  "Disables an experimental new pass manager in LLVM.">, Group<f_clang_Group>, Flags<[CC1Option]>;
+defm legacy_pass_manager : BooleanMarshalledFFlag<"legacy-pass-manager", "CodeGenOpts.LegacyPassManager",
+  "static_cast<unsigned>(LLVM_ENABLE_NEW_PASS_MANAGER)", "Use the legacy pass manager in LLVM",
+  "Use the new pass manager in LLVM">, Group<f_clang_Group>, Flags<[CC1Option]>;
+def fexperimental_new_pass_manager : Flag<["-"], "fexperimental-new-pass-manager">,
+  Group<f_clang_Group>, Flags<[CC1Option]>, Alias<fno_legacy_pass_manager>;
+def fno_experimental_new_pass_manager : Flag<["-"], "fno-experimental-new-pass-manager">,
+  Group<f_clang_Group>, Flags<[CC1Option]>, Alias<flegacy_pass_manager>;
 def fexperimental_strict_floating_point : Flag<["-"], "fexperimental-strict-floating-point">,
   Group<f_clang_Group>, Flags<[CC1Option]>,
   HelpText<"Enables experimental strict floating point in LLVM.">,
   MarshallingInfoFlag<"LangOpts->ExpStrictFP">;
 def finput_charset_EQ : Joined<["-"], "finput-charset=">, Group<f_Group>;
 def fexec_charset_EQ : Joined<["-"], "fexec-charset=">, Group<f_Group>;
 def finstrument_functions : Flag<["-"], "finstrument-functions">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Generate calls to instrument function entry and exit">;
 def finstrument_functions_after_inlining : Flag<["-"], "finstrument-functions-after-inlining">, Group<f_Group>, Flags<[CC1Option]>,

clang/lib/CodeGen/BackendUtil.cpp

if (CGOpts.hasProfileCSIRInstr()) {
     Conf.RunCSIRInstr = true;
     Conf.CSIRProfile = std::move(CGOpts.InstrProfileOutput);
   } else if (CGOpts.hasProfileCSIRUse()) {
     Conf.RunCSIRInstr = false;
     Conf.CSIRProfile = std::move(CGOpts.ProfileInstrumentUsePath);
   }
 
   Conf.ProfileRemapping = std::move(ProfileRemapping);
-  Conf.UseNewPM = CGOpts.ExperimentalNewPassManager;
+  Conf.UseNewPM = !CGOpts.LegacyPassManager;
   Conf.DebugPassManager = CGOpts.DebugPassManager;
   Conf.RemarksWithHotness = CGOpts.DiagnosticsWithHotness;
   Conf.RemarksFilename = CGOpts.OptRecordFile;
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
   Conf.RemarksFormat = CGOpts.OptRecordFormat;
   Conf.SplitDwarfFile = CGOpts.SplitDwarfFile;
   Conf.SplitDwarfOutput = CGOpts.SplitDwarfOutput;
   switch (Action) {
if (CombinedIndex) {
       EmptyModule = std::make_unique<llvm::Module>("empty", M->getContext());
       EmptyModule->setTargetTriple(M->getTargetTriple());
       M = EmptyModule.get();
     }
   }
 
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
-  if (CGOpts.ExperimentalNewPassManager)
+  if (!CGOpts.LegacyPassManager)
     AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
     AsmHelper.EmitAssembly(Action, std::move(OS));
 
   // Verify clang's TargetInfo DataLayout against the LLVM TargetMachine's
   // DataLayout.
   if (AsmHelper.TM) {
     std::string DLDesc = M->getDataLayout().getStringRepresentation();

clang/lib/Driver/ToolChains/Clang.cpp

if (Args.hasFlag(options::OPT_fpch_instantiate_templates,
     CmdArgs.push_back("-fpch-instantiate-templates");
   if (Args.hasFlag(options::OPT_fpch_codegen, options::OPT_fno_pch_codegen,
                    false))
     CmdArgs.push_back("-fmodules-codegen");
   if (Args.hasFlag(options::OPT_fpch_debuginfo, options::OPT_fno_pch_debuginfo,
                    false))
     CmdArgs.push_back("-fmodules-debuginfo");
 
-  Args.AddLastArg(CmdArgs, options::OPT_fexperimental_new_pass_manager,
-                  options::OPT_fno_experimental_new_pass_manager);
+  Args.AddLastArg(CmdArgs, options::OPT_flegacy_pass_manager,
+                  options::OPT_fno_legacy_pass_manager);
 
   ObjCRuntime Runtime = AddObjCRuntimeArgs(Args, Inputs, CmdArgs, rewriteKind);
   RenderObjCOptions(TC, D, RawTriple, Args, Runtime, rewriteKind != RK_None,
                     Input, CmdArgs);
 
   if (Args.hasFlag(options::OPT_fapplication_extension,
                    options::OPT_fno_application_extension, false))
     CmdArgs.push_back("-fapplication-extension");

clang/lib/Driver/ToolChains/CommonArgs.cpp

if (CSPGOGenerateArg) {
     SmallString<128> Path(
         ProfileUseArg->getNumValues() == 0 ? "" : ProfileUseArg->getValue());
     if (Path.empty() || llvm::sys::fs::is_directory(Path))
       llvm::sys::path::append(Path, "default.profdata");
     CmdArgs.push_back(Args.MakeArgString(Twine("-plugin-opt=cs-profile-path=") +
                                          Path));
   }
 
-  // Need this flag to turn on new pass manager via Gold plugin.
-  if (Args.hasFlag(options::OPT_fexperimental_new_pass_manager,
-                   options::OPT_fno_experimental_new_pass_manager,
-                   /* Default */ LLVM_ENABLE_NEW_PASS_MANAGER)) {
+  // Pass an option to enable/disable the new pass manager.
+  if (auto *A = Args.getLastArg(options::OPT_flegacy_pass_manager,
+                                options::OPT_fno_legacy_pass_manager)) {
+    if (A->getOption().matches(options::OPT_flegacy_pass_manager))
+      CmdArgs.push_back("-plugin-opt=no-new-pass-manager");
+    else
       CmdArgs.push_back("-plugin-opt=new-pass-manager");
   }
 
   // Setup statistics file output.
   SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
   if (!StatsFile.empty())
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-plugin-opt=stats-file=") + StatsFile));
 

clang/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.InstrProfileOutput =
       std::string(Args.getLastArgValue(OPT_fprofile_instrument_path_EQ));
   Opts.ProfileInstrumentUsePath =
       std::string(Args.getLastArgValue(OPT_fprofile_instrument_use_path_EQ));
   if (!Opts.ProfileInstrumentUsePath.empty())
     setPGOUseInstrumentor(Opts, Opts.ProfileInstrumentUsePath);
   Opts.ProfileRemappingFile =
       std::string(Args.getLastArgValue(OPT_fprofile_remapping_file_EQ));
-  if (!Opts.ProfileRemappingFile.empty() && !Opts.ExperimentalNewPassManager) {
+  if (!Opts.ProfileRemappingFile.empty() && Opts.LegacyPassManager) {
     Diags.Report(diag::err_drv_argument_only_allowed_with)
         << Args.getLastArg(OPT_fprofile_remapping_file_EQ)->getAsString(Args)
-      << "-fexperimental-new-pass-manager";
+        << "-fno-legacy-pass-manager";
   }
 
   Opts.CoverageMapping =
       Args.hasFlag(OPT_fcoverage_mapping, OPT_fno_coverage_mapping, false);
   Opts.DumpCoverageMapping = Args.hasArg(OPT_dump_coverage_mapping);
   Opts.AsmVerbose = !Args.hasArg(OPT_fno_verbose_asm);
   Opts.PreserveAsmComments = !Args.hasArg(OPT_fno_preserve_as_comments);
   Opts.AssumeSaneOperatorNew = !Args.hasArg(OPT_fno_assume_sane_operator_new);
if (ValidFP)
       Opts.setFramePointer(FP);
   }
 
   if (const Arg *A = Args.getLastArg(OPT_ftime_report, OPT_ftime_report_EQ)) {
     Opts.TimePasses = true;
 
     // -ftime-report= is only for new pass manager.
     if (A->getOption().getID() == OPT_ftime_report_EQ) {
-      if (!Opts.ExperimentalNewPassManager)
+      if (Opts.LegacyPassManager)
         Diags.Report(diag::err_drv_argument_only_allowed_with)
-            << A->getAsString(Args) << "-fexperimental-new-pass-manager";
+            << A->getAsString(Args) << "-fno-legacy-pass-manager";
 
       StringRef Val = A->getValue();
       if (Val == "per-pass")
         Opts.TimePassesPerRun = false;
       else if (Val == "per-pass-run")
         Opts.TimePassesPerRun = true;
       else
         Diags.Report(diag::err_drv_invalid_value)

clang/test/Driver/clang_f_opts.c

 // Delimiters match the start of the cc1 and the start of the linker lines
 // DELIMITERS: {{^ (\(in-process\)|")}}
 // CHECK-WCHAR1: -fwchar-type=int
 // CHECK-WCHAR1-NOT: -fwchar-type=short
 // CHECK-WCHAR2: -fwchar-type=short
 // CHECK-WCHAR2-NOT: -fwchar-type=int
 // DELIMITERS: {{^ *"}}
 
-// RUN: %clang -### -fno-experimental-new-pass-manager -fexperimental-new-pass-manager %s 2>&1 | FileCheck --check-prefix=CHECK-PM --check-prefix=CHECK-NEW-PM %s
-// RUN: %clang -### -fexperimental-new-pass-manager -fno-experimental-new-pass-manager %s 2>&1 | FileCheck --check-prefix=CHECK-PM --check-prefix=CHECK-NO-NEW-PM %s
-// CHECK-PM-NOT: argument unused
-// CHECK-NEW-PM: -fexperimental-new-pass-manager
-// CHECK-NEW-PM-NOT: -fno-experimental-new-pass-manager
-// CHECK-NO-NEW-PM: -fno-experimental-new-pass-manager
-// CHECK-NO-NEW-PM-NOT: -fexperimental-new-pass-manager
-
 // RUN: %clang -### -S -fstrict-return %s 2>&1 | FileCheck -check-prefix=CHECK-STRICT-RETURN %s
 // RUN: %clang -### -S -fno-strict-return %s 2>&1 | FileCheck -check-prefix=CHECK-NO-STRICT-RETURN %s
 // CHECK-STRICT-RETURN-NOT: "-fno-strict-return"
 // CHECK-NO-STRICT-RETURN: "-fno-strict-return"
 
 // RUN: %clang -### -S -fno-debug-info-for-profiling -fdebug-info-for-profiling %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-DEBUG %s
 // RUN: %clang -### -S -fdebug-info-for-profiling -fno-debug-info-for-profiling %s 2>&1 | FileCheck -check-prefix=CHECK-NO-PROFILE-DEBUG %s
 // CHECK-PROFILE-DEBUG: -fdebug-info-for-profiling

clang/test/Driver/flegacy-pass-manager.c

+// RUN: %clang -### -c -flegacy-pass-manager -fno-legacy-pass-manager %s 2>&1 | FileCheck --check-prefixes=NOWARN,NEW %s
+// RUN: %clang -### -c -fno-legacy-pass-manager -flegacy-pass-manager %s 2>&1 | FileCheck --check-prefixes=NOWARN,LEGACY %s
+
+/// -f[no-]experimental-new-pass-manager are legacy aliases when the new PM was still experimental.
+// RUN: %clang -### -c -fno-experimental-new-pass-manager -fexperimental-new-pass-manager %s 2>&1 | FileCheck --check-prefixes=NOWARN,NEW %s
+// RUN: %clang -### -c -fexperimental-new-pass-manager -fno-experimental-new-pass-manager %s 2>&1 | FileCheck --check-prefixes=NOWARN,LEGACY %s
+
+// NOWARN-NOT: warning: argument unused
+
+// NEW:        -fno-legacy-pass-manager
+// NEW-NOT:    -flegacy-pass-manager
+
+// LEGACY:     -flegacy-pass-manager
+// LEGACY-NOT: -fno-legacy-pass-manager
+
+/// For full/Thin LTO, -fno-legacy-pass-manager passes -plugin-opt=new-pass-manager to the linker (which may not be LLD).
+// RUN: %clang -### -target x86_64-linux -flto -fno-legacy-pass-manager %s 2>&1 | FileCheck --check-prefix=LTO_NEW %s
+// RUN: %clang -### -target x86_64-linux -flto=thin -fexperimental-new-pass-manager %s 2>&1 | FileCheck --check-prefix=LTO_NEW %s
+
+// LTO_NEW:    "-plugin-opt=new-pass-manager"
+
+// RUN: %clang -### -target x86_64-linux -flto -flegacy-pass-manager %s 2>&1 | FileCheck --check-prefix=LTO_LEGACY %s
+// RUN: %clang -### -target x86_64-linux -flto=thin -fno-experimental-new-pass-manager %s 2>&1 | FileCheck --check-prefix=LTO_LEGACY %s
+
+// LTO_LEGACY: "-plugin-opt=no-new-pass-manager"
+
+// RUN: %clang -### -target x86_64-linux -flto %s 2>&1 | FileCheck --check-prefix=DEFAULT %s
+//
+// DEFAULT-NOT: "-plugin-opt=new-pass-manager"
+// DEFAULT-NOT: "-plugin-opt=no-new-pass-manager"

clang/test/Driver/gold-lto-new-pass-man.c

-// RUN: touch %t.o
-//
-// RUN: %clang -target ppc64le-unknown-linux -### %t.o -flto 2>&1 \
-// RUN:     -Wl,-plugin-opt=foo -O3 \
-// RUN:     -fexperimental-new-pass-manager \
-// RUN:     | FileCheck %s
-// CHECK: "-plugin-opt=new-pass-manager"