[clang][driver] Honor the last -flto(=.*)? argument

[mnadeem]

Summary

1. After D102479 code in Clang.cpp only propagates -flto instead of -flto=thin if we pass -flto -flto=thin to clang, ignoring the last -flto=.* argument.

2. I also noticed that parseLTOMode returns LTOK_Thin when we pass -flto=thin -flto. Using the -flto= argument even though a later -flto is present.

[mnadeem]

clang/lib/Driver/ToolChains/Clang.cpp

4476–4477

Another option would be to do the following, but i am not sure if there is any code that explicitly checks for/needs "=full":

if (D.getLTOMode() == LTOK_Thin)
    CmdArgs.push_back("-flto=thin");
else
    CmdArgs.push_back("-flto");

[tbaeder]

That code has changed quite a bit since I've worked on it.

The only problem I could see is that passing -flto=thin -flto and choosing thin LTO kinda makes sense if you interpret -flto as just "use LTO". The -flto=thin is more specific, so it could make sense to pick that. But that's just something I wonder while looking at it. Not sure if we process other options like this by just choosing the last one.

[tejohnson]

In D108881#2971651, @tbaeder wrote:

That code has changed quite a bit since I've worked on it.

The only problem I could see is that passing -flto=thin -flto and choosing thin LTO kinda makes sense if you interpret -flto as just "use LTO". The -flto=thin is more specific, so it could make sense to pick that. But that's just something I wonder while looking at it. Not sure if we process other options like this by just choosing the last one.

The default of -flto is documented as -flto=full, so I think the change here makes sense. In general the last option wins.

clang/lib/Driver/ToolChains/Clang.cpp

4476–4477

Looks like the result of getLTOMode() is already saved in the LTOMode local variable.

4476–4477

Yes, that should be fine since the default is full. But better yet, make the else pass the explicit -flto=full.

4477

Can we consolidate the option handling into parseLTOMode, which does the rest of the -flto option handling and sets the LTOMode returned by the below call to getLTOMode()? The default of -flto is "full" so it can be treated there as such. Actually, looks like it should already be doing this?

clang/test/Driver/lto.c

97

Probably want to check that we don't have additional -flto options being passed too, here and on the below lines.

[mnadeem]

Address comments.

[mnadeem]

Combined with the code in else if (Triple.isAMDGPU()) since the code is identical.
Removed the code to add -flto in favour of -flto=full since both are equivalent: https://clang.llvm.org/docs/CommandGuide/clang.html#cmdoption-flto

[steven_wu]

I agree that it might be good to change to the behavior that last arg wins but it might be simpler to set -flto to be an alias of -flto=full:

--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2086,7 +2086,7 @@ def flto_EQ : Joined<["-"], "flto=">, Flags<[CoreOption, CC1Option]>, Group<f_Gr
 def flto_EQ_jobserver : Flag<["-"], "flto=jobserver">, Group<f_Group>;
 def flto_EQ_auto : Flag<["-"], "flto=auto">, Group<f_Group>;
 def flto : Flag<["-"], "flto">, Flags<[CoreOption, CC1Option]>, Group<f_Group>,
-  HelpText<"Enable LTO in 'full' mode">;
+  Alias<flto_EQ>, AliasArgs<["full"]>, HelpText<"Enable LTO in 'full' mode">;
 def fno_lto : Flag<["-"], "fno-lto">, Flags<[CoreOption, CC1Option]>, Group<f_Group>,
   HelpText<"Disable LTO mode (default)">;
 def foffload_lto_EQ : Joined<["-"], "foffload-lto=">, Flags<[CoreOption]>, Group<f_Group>,

Of course, you want to clean up the option processing of flto.

[mnadeem]

Update Options.td as suggested by steven_wu

[steven_wu]

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

[tejohnson]

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Good idea on simplifying via an alias, agree that this should all just be cleaned up now. Thanks

[mnadeem]

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

[steven_wu]

In D108881#2973719, @mnadeem wrote:

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

The current change set in this review is functional change while the cleanup I want is not functional after the rewrite the old option as Alias. Once flto is the alias, there is no need to handle that in the driver and those might actually become source of bug in the future.

I think it would be good to do the cleanup in the same commit unless you have compelling reason not to.

[mnadeem]

In D108881#2973735, @steven_wu wrote:

In D108881#2973719, @mnadeem wrote:

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

The current change set in this review is functional change while the cleanup I want is not functional after the rewrite the old option as Alias. Once flto is the alias, there is no need to handle that in the driver and those might actually become source of bug in the future.

I think it would be good to do the cleanup in the same commit unless you have compelling reason not to.

Hi @steven_wu any idea about the timeline?

This issue is blocking some internal work, and assuming that it will take longer to get a full fix, I would prefer it if this change could go in on its own.
Otherwise I am good with doing everything in the same commit.

[steven_wu]

In D108881#2987990, @mnadeem wrote:

In D108881#2973735, @steven_wu wrote:

In D108881#2973719, @mnadeem wrote:

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

The current change set in this review is functional change while the cleanup I want is not functional after the rewrite the old option as Alias. Once flto is the alias, there is no need to handle that in the driver and those might actually become source of bug in the future.

I think it would be good to do the cleanup in the same commit unless you have compelling reason not to.

Hi @steven_wu any idea about the timeline?

This issue is blocking some internal work, and assuming that it will take longer to get a full fix, I would prefer it if this change could go in on its own.
Otherwise I am good with doing everything in the same commit.

I expect the cleanup is very very simple and that is why I am suggested to do in the same commit. I am ok with a followup fix as well.

[steven_wu]

If you didn't quite get what I mean, I am suggesting to cleanup all the reference to the old flag. Since -flto is rewrite by the driver to -flto=full, clang should not query OPT_flto anymore. All references to the old flag needs to be audited and removed while preserving the correct behavior.

[tejohnson]

In D108881#2988016, @steven_wu wrote:

In D108881#2987990, @mnadeem wrote:

In D108881#2973735, @steven_wu wrote:

In D108881#2973719, @mnadeem wrote:

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

The current change set in this review is functional change while the cleanup I want is not functional after the rewrite the old option as Alias. Once flto is the alias, there is no need to handle that in the driver and those might actually become source of bug in the future.

I think it would be good to do the cleanup in the same commit unless you have compelling reason not to.

Hi @steven_wu any idea about the timeline?

This issue is blocking some internal work, and assuming that it will take longer to get a full fix, I would prefer it if this change could go in on its own.
Otherwise I am good with doing everything in the same commit.

I expect the cleanup is very very simple and that is why I am suggested to do in the same commit. I am ok with a followup fix as well.

I think there might be some high level confusion. @steven_wu earlier in the thread you said "I will do a cleanup", but I think you were asking @mnadeem to do the cleanup here in this patch.

[steven_wu]

In D108881#2988026, @tejohnson wrote:

In D108881#2988016, @steven_wu wrote:

In D108881#2987990, @mnadeem wrote:

In D108881#2973735, @steven_wu wrote:

In D108881#2973719, @mnadeem wrote:

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

The current change set in this review is functional change while the cleanup I want is not functional after the rewrite the old option as Alias. Once flto is the alias, there is no need to handle that in the driver and those might actually become source of bug in the future.

I think it would be good to do the cleanup in the same commit unless you have compelling reason not to.

Hi @steven_wu any idea about the timeline?

This issue is blocking some internal work, and assuming that it will take longer to get a full fix, I would prefer it if this change could go in on its own.
Otherwise I am good with doing everything in the same commit.

I expect the cleanup is very very simple and that is why I am suggested to do in the same commit. I am ok with a followup fix as well.

I think there might be some high level confusion. @steven_wu earlier in the thread you said "I will do a cleanup", but I think you were asking @mnadeem to do the cleanup here in this patch.

Ah, I see. I probably type the reply from my phone so I didn't double check. I really mean "I would suggest @mnadeem to do a cleanup in the same commit". Sorry about the confusion.

[mnadeem]

• - Make flto an alias of flto=full.
• - Make foffload-lto an alias of foffload-lto=full.
• - Make flto_EQ_jobserver, flto_EQ_auto aliases of flto=full, since they are being treated as full lto right now.
• - Clean up the code for parseLTOMode and setLTOMode.
• - Replace uses of OPT_flto with OPT_flto_EQ.

I'm not very familiar with driver code so let me know if I made any mistake or left anything out.

[mnadeem]

clang/lib/Driver/Driver.cpp

596–598

These were removed because the options now alias with flto=full.

[steven_wu]

I am not familiar with offloading side of LTO driver but the change looks functionally the same to me. Thanks for doing this! Remember to update the commit message with the correct info.

clang/include/clang/Driver/Options.td

MarshallingInfoEnum<LangOpts<"LaxVectorConversions">,
                           " : LangOptions::LaxVectorConversionKind::All">;
 def flax_vector_conversions : Flag<["-"], "flax-vector-conversions">, Group<f_Group>,
   Alias<flax_vector_conversions_EQ>, AliasArgs<["integer"]>;
 def flimited_precision_EQ : Joined<["-"], "flimited-precision=">, Group<f_Group>;
 def fapple_link_rtlib : Flag<["-"], "fapple-link-rtlib">, Group<f_Group>,
   HelpText<"Force linking the clang builtins runtime library">;
 def flto_EQ : Joined<["-"], "flto=">, Flags<[CoreOption, CC1Option]>, Group<f_Group>,
   HelpText<"Set LTO mode to either 'full' or 'thin'">, Values<"thin,full">;
-def flto_EQ_jobserver : Flag<["-"], "flto=jobserver">, Group<f_Group>;
-def flto_EQ_auto : Flag<["-"], "flto=auto">, Group<f_Group>;
+def flto_EQ_jobserver : Flag<["-"], "flto=jobserver">, Group<f_Group>,
+  Alias<flto_EQ>, AliasArgs<["full"]>, HelpText<"Enable LTO in 'full' mode">;
+def flto_EQ_auto : Flag<["-"], "flto=auto">, Group<f_Group>,
+  Alias<flto_EQ>, AliasArgs<["full"]>, HelpText<"Enable LTO in 'full' mode">;
 def flto : Flag<["-"], "flto">, Flags<[CoreOption, CC1Option]>, Group<f_Group>,
-  HelpText<"Enable LTO in 'full' mode">;
+  Alias<flto_EQ>, AliasArgs<["full"]>, HelpText<"Enable LTO in 'full' mode">;
 def fno_lto : Flag<["-"], "fno-lto">, Flags<[CoreOption, CC1Option]>, Group<f_Group>,
   HelpText<"Disable LTO mode (default)">;
 def foffload_lto_EQ : Joined<["-"], "foffload-lto=">, Flags<[CoreOption]>, Group<f_Group>,
   HelpText<"Set LTO mode to either 'full' or 'thin' for offload compilation">, Values<"thin,full">;
 def foffload_lto : Flag<["-"], "foffload-lto">, Flags<[CoreOption]>, Group<f_Group>,
-  HelpText<"Enable LTO in 'full' mode for offload compilation">;
+  Alias<foffload_lto_EQ>, AliasArgs<["full"]>, HelpText<"Enable LTO in 'full' mode for offload compilation">;
 def fno_offload_lto : Flag<["-"], "fno-offload-lto">, Flags<[CoreOption]>, Group<f_Group>,
   HelpText<"Disable LTO mode (default) for offload compilation">;
 def flto_jobs_EQ : Joined<["-"], "flto-jobs=">,
   Flags<[CC1Option]>, Group<f_Group>,
   HelpText<"Controls the backend parallelism of -flto=thin (default "
            "of 0 means the number of threads will be derived from "
            "the number of CPUs detected)">;
 def fthinlto_index_EQ : Joined<["-"], "fthinlto-index=">,

clang/lib/Driver/Driver.cpp

static llvm::Triple computeTargetTriple(const Driver &D,
   }
 
   return Target;
 }
 
 // Parse the LTO options and record the type of LTO compilation
 // based on which -f(no-)?lto(=.*)? or -f(no-)?offload-lto(=.*)?
 // option occurs last.
-static llvm::Optional<driver::LTOKind>
-parseLTOMode(Driver &D, const llvm::opt::ArgList &Args, OptSpecifier OptPos,
-             OptSpecifier OptNeg, OptSpecifier OptEq, bool IsOffload) {
-  driver::LTOKind LTOMode = LTOK_None;
-  // Non-offload LTO allows -flto=auto and -flto=jobserver. Offload LTO does
-  // not support those options.
-  if (!Args.hasFlag(OptPos, OptEq, OptNeg, false) &&
-      (IsOffload ||
-       (!Args.hasFlag(options::OPT_flto_EQ_auto, options::OPT_fno_lto, false) &&
-        !Args.hasFlag(options::OPT_flto_EQ_jobserver, options::OPT_fno_lto,
-                      false))))
-    return None;
-
-  StringRef LTOName("full");
+static driver::LTOKind parseLTOMode(Driver &D, const llvm::opt::ArgList &Args,
+                                    OptSpecifier OptEq, OptSpecifier OptNeg) {
+  if (!Args.hasFlag(OptEq, OptNeg, false))
+    return LTOK_None;
 
   const Arg *A = Args.getLastArg(OptEq);
-  if (A)
-    LTOName = A->getValue();
+  StringRef LTOName = A->getValue();
 
-  LTOMode = llvm::StringSwitch<LTOKind>(LTOName)
+  driver::LTOKind LTOMode = llvm::StringSwitch<LTOKind>(LTOName)
                                 .Case("full", LTOK_Full)
                                 .Case("thin", LTOK_Thin)
                                 .Default(LTOK_Unknown);
 
   if (LTOMode == LTOK_Unknown) {
-    assert(A);
     D.Diag(diag::err_drv_unsupported_option_argument)
         << A->getOption().getName() << A->getValue();
-    return None;
+    return LTOK_None;
   }
   return LTOMode;
 }
 
 // Parse the LTO options.
 void Driver::setLTOMode(const llvm::opt::ArgList &Args) {
-  LTOMode = LTOK_None;
-  if (auto M = parseLTOMode(*this, Args, options::OPT_flto,
-                            options::OPT_fno_lto, options::OPT_flto_EQ,
-                            /*IsOffload=*/false))
-    LTOMode = M.getValue();
-
-  OffloadLTOMode = LTOK_None;
-  if (auto M = parseLTOMode(*this, Args, options::OPT_foffload_lto,
-                            options::OPT_fno_offload_lto,
-                            options::OPT_foffload_lto_EQ,
-                            /*IsOffload=*/true))
-    OffloadLTOMode = M.getValue();
+  LTOMode =
+      parseLTOMode(*this, Args, options::OPT_flto_EQ, options::OPT_fno_lto);
+
+  OffloadLTOMode = parseLTOMode(*this, Args, options::OPT_foffload_lto_EQ,
+                                options::OPT_fno_offload_lto);
 }
 
 /// Compute the desired OpenMP runtime from the flags provided.
 Driver::OpenMPRuntimeKind Driver::getOpenMPRuntime(const ArgList &Args) const {
   StringRef RuntimeName(CLANG_DEFAULT_OPENMP_RUNTIME);
 
   const Arg *A = Args.getLastArg(options::OPT_fopenmp_EQ);
   if (A)

clang/lib/Driver/ToolChains/Clang.cpp

if (isa<AnalyzeJobAction>(JA)) {
 
     // Preserve use-list order by default when emitting bitcode, so that
     // loading the bitcode up in 'opt' or 'llc' and running passes gives the
     // same result as running passes here.  For LTO, we don't need to preserve
     // the use-list order, since serialization to bitcode is part of the flow.
     if (JA.getType() == types::TY_LLVM_BC)
       CmdArgs.push_back("-emit-llvm-uselists");
 
     if (IsUsingLTO) {
-      if (!IsDeviceOffloadAction) {
-        if (Args.hasArg(options::OPT_flto))
-          CmdArgs.push_back("-flto");
-        else {
-          if (D.getLTOMode() == LTOK_Thin)
-            CmdArgs.push_back("-flto=thin");
-          else
-            CmdArgs.push_back("-flto=full");
-        }
-        CmdArgs.push_back("-flto-unit");
-      } else if (Triple.isAMDGPU()) {
-        // Only AMDGPU supports device-side LTO
-        assert(LTOMode == LTOK_Full || LTOMode == LTOK_Thin);
-        CmdArgs.push_back(Args.MakeArgString(
-            Twine("-flto=") + (LTOMode == LTOK_Thin ? "thin" : "full")));
-        CmdArgs.push_back("-flto-unit");
-      } else {
+      // Only AMDGPU supports device-side LTO.
+      if (IsDeviceOffloadAction && !Triple.isAMDGPU()) {
         D.Diag(diag::err_drv_unsupported_opt_for_target)
             << Args.getLastArg(options::OPT_foffload_lto,
                                options::OPT_foffload_lto_EQ)
                    ->getAsString(Args)
             << Triple.getTriple();
+      } else {
+        assert(LTOMode == LTOK_Full || LTOMode == LTOK_Thin);
+        CmdArgs.push_back(Args.MakeArgString(
+            Twine("-flto=") + (LTOMode == LTOK_Thin ? "thin" : "full")));
+        CmdArgs.push_back("-flto-unit");
       }
     }
   }
 
   if (const Arg *A = Args.getLastArg(options::OPT_fthinlto_index_EQ)) {
     if (!types::isLLVMIR(Input.getType()))
       D.Diag(diag::err_drv_arg_requires_bitcode_input) << A->getAsString(Args);
     Args.AddLastArg(CmdArgs, options::OPT_fthinlto_index_EQ);

clang/lib/Frontend/CompilerInvocation.cpp

#undef CODEGEN_OPTION_WITH_MARSHALLING
   if (Opts.TimePasses) {
     if (Opts.TimePassesPerRun)
       GenerateArg(Args, OPT_ftime_report_EQ, "per-pass-run", SA);
     else
       GenerateArg(Args, OPT_ftime_report, SA);
   }
 
   if (Opts.PrepareForLTO && !Opts.PrepareForThinLTO)
-    GenerateArg(Args, OPT_flto, SA);
+    GenerateArg(Args, OPT_flto_EQ, "full", SA);
 
   if (Opts.PrepareForThinLTO)
     GenerateArg(Args, OPT_flto_EQ, "thin", SA);
 
   if (!Opts.ThinLTOIndexFile.empty())
     GenerateArg(Args, OPT_fthinlto_index_EQ, Opts.ThinLTOIndexFile, SA);
 
   if (Opts.SaveTempsFilePrefix == OutputFile)
if (A->getOption().getID() == OPT_ftime_report_EQ) {
       else if (Val == "per-pass-run")
         Opts.TimePassesPerRun = true;
       else
         Diags.Report(diag::err_drv_invalid_value)
             << A->getAsString(Args) << A->getValue();
     }
   }
 
-  Opts.PrepareForLTO = Args.hasArg(OPT_flto, OPT_flto_EQ);
+  Opts.PrepareForLTO = false;
   Opts.PrepareForThinLTO = false;
   if (Arg *A = Args.getLastArg(OPT_flto_EQ)) {
+    Opts.PrepareForLTO = true;
     StringRef S = A->getValue();
     if (S == "thin")
       Opts.PrepareForThinLTO = true;
     else if (S != "full")
       Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << S;
   }
   if (Arg *A = Args.getLastArg(OPT_fthinlto_index_EQ)) {
     if (IK.getLanguage() != Language::LLVM_IR)

clang/test/Driver/amdgpu-toolchain.c

 // RUN: %clang -### -target amdgcn--amdhsa -x assembler -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=AS_LINK %s
 // RUN: %clang -### -g -target amdgcn--amdhsa -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=DWARF_VER %s
 // RUN: %clang -### -target amdgcn-amd-amdpal -x assembler -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=AS_LINK %s
 // RUN: %clang -### -g -target amdgcn-amd-amdpal -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=DWARF_VER %s
 // RUN: %clang -### -target amdgcn-mesa-mesa3d -x assembler -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=AS_LINK %s
 // RUN: %clang -### -g -target amdgcn-mesa-mesa3d -mcpu=kaveri %s 2>&1 | FileCheck -check-prefix=DWARF_VER %s
 
 // AS_LINK: clang{{.*}} "-cc1as"
 // AS_LINK: ld.lld{{.*}} "-shared"
 
 // DWARF_VER: "-dwarf-version=5"
 
 // RUN: %clang -### -target amdgcn-amd-amdhsa -mcpu=gfx906 -nogpulib \
 // RUN:   -flto %s 2>&1 | FileCheck -check-prefix=LTO %s
-// LTO: clang{{.*}} "-flto"
+// LTO: clang{{.*}} "-flto=full"
 // LTO: ld.lld{{.*}}

clang/test/Driver/lto.c

 //
 // -flto=auto and -flto=jobserver pass along -flto=full
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto=auto 2>&1 | FileCheck --check-prefix=FLTO-AUTO %s
 // RUN: %clang -target x86_64-unknown-linux -### %s -flto=jobserver 2>&1 | FileCheck --check-prefix=FLTO-JOBSERVER %s
 //
 // FLTO-AUTO: -flto=full
 // FLTO-JOBSERVER: -flto=full
 //
+
+// Pass the last -flto argument.
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -flto 2>&1 | \
+// RUN: FileCheck --check-prefix=FLTO-FULL %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=thin -flto=full \
+// RUN: 2>&1 | FileCheck --check-prefix=FLTO-FULL %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto=full -flto=thin  \
+// RUN: 2>&1 | FileCheck --check-prefix=FLTO-THIN %s
+// RUN: %clang -target x86_64-unknown-linux -### %s -flto -flto=thin 2>&1 | \
+// RUN: FileCheck --check-prefix=FLTO-THIN %s
+//
+// FLTO-FULL-NOT: -flto=thin
+// FLTO-FULL: -flto=full
+// FLTO-FULL-NOT: -flto=thin
+//
+// FLTO-THIN-NOT: -flto=full
+// FLTO-THIN-NOT: "-flto"
+// FLTO-THIN: -flto=thin
+// FLTO-THIN-NOT: "-flto"
+// FLTO-THIN-NOT: -flto=full
 No newline at end of file