[PATCH] D108881: [clang][driver] Honor the last -flto= flag even if an earlier -flto is present

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D108881#2971651 <https://reviews.llvm.org/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.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4495-4497
+        Arg *A = Args.getLastArg(options::OPT_flto, options::OPT_flto_EQ);
+        if (A && A->getOption().matches(options::OPT_flto))
           CmdArgs.push_back("-flto");
----------------
mnadeem wrote:
> 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");
> ```
Yes, that should be fine since the default is full. But better yet, make the else pass the explicit -flto=full.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4496
+        Arg *A = Args.getLastArg(options::OPT_flto, options::OPT_flto_EQ);
+        if (A && A->getOption().matches(options::OPT_flto))
           CmdArgs.push_back("-flto");
----------------
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?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4498
           CmdArgs.push_back("-flto");
-        else {
-          if (D.getLTOMode() == LTOK_Thin)
-            CmdArgs.push_back("-flto=thin");
-          else
-            CmdArgs.push_back("-flto=full");
-        }
+        else if (D.getLTOMode() == LTOK_Thin)
+          CmdArgs.push_back("-flto=thin");
----------------
Looks like the result of getLTOMode() is already saved in the LTOMode local variable.


================
Comment at: clang/test/Driver/lto.c:97
+//
+// FLTO-THIN: -flto=thin
+// FLTO-FULL: -flto=full
----------------
Probably want to check that we don't have additional -flto options being passed too, here and on the below lines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108881/new/

https://reviews.llvm.org/D108881