[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

4493–4494

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

4493–4494

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

4493–4494

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

4494

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 ↗

(On Diff #371241)

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

 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 : 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/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
+      // Only AMDGPU supports device-side LTO.
+      if (!IsDeviceOffloadAction || Triple.isAMDGPU()) {
         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 {
         D.Diag(diag::err_drv_unsupported_opt_for_target)
             << Args.getLastArg(options::OPT_foffload_lto,
                                options::OPT_foffload_lto_EQ)

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 2>&1 | \
+// RUN: FileCheck --check-prefix=FLTO-FULL %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