[Driver][LTO] Move common code for LTO to addLTOOptions()

[brad0]

No description provided.

[llvmbot]

@llvm/pr-subscribers-backend-powerpc
@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Brad Smith (brad0)

Changes

Copying 1881832 to the last of the targets that use LTO.

But I am not sure about tests for these targets, especially the AMD toolchains.

Could the respective AMD and MinGW commiters please help here?

---

Full diff: https://github.com/llvm/llvm-project/pull/74178.diff

3 Files Affected:

• (modified) clang/lib/Driver/ToolChains/AMDGPU.cpp (+12-3)
• (modified) clang/lib/Driver/ToolChains/HIPAMD.cpp (+9-1)
• (modified) clang/lib/Driver/ToolChains/MinGW.cpp (+9-1)

diff --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp b/clang/lib/Driver/ToolChains/AMDGPU.cpp
index cad206ea4df1bc5..3776d3bbac811b9 100644
--- a/clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -611,10 +611,19 @@ void amdgpu::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   addLinkerCompressDebugSectionsOption(getToolChain(), Args, CmdArgs);
   Args.AddAllArgs(CmdArgs, options::OPT_L);
   AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA);
-  if (C.getDriver().isUsingLTO())
-    addLTOOptions(getToolChain(), Args, CmdArgs, Output, Inputs[0],
+  if (C.getDriver().isUsingLTO()) {
+    assert(!Inputs.empty() && "Must have at least one input.");
+    // Find the first filename InputInfo object.
+    auto Input = llvm::find_if(
+        Inputs, [](const InputInfo &II) -> bool { return II.isFilename(); });
+    if (Input == Inputs.end())
+      // For a very rare case, all of the inputs to the linker are
+      // InputArg. If that happens, just use the first InputInfo.
+      Input = Inputs.begin();
+
+    addLTOOptions(getToolChain(), Args, CmdArgs, Output, *Input,
                   C.getDriver().getLTOMode() == LTOK_Thin);
-  else if (Args.hasArg(options::OPT_mcpu_EQ))
+  } else if (Args.hasArg(options::OPT_mcpu_EQ))
     CmdArgs.push_back(Args.MakeArgString(
         "-plugin-opt=mcpu=" + Args.getLastArgValue(options::OPT_mcpu_EQ)));
   CmdArgs.push_back("-o");
diff --git a/clang/lib/Driver/ToolChains/HIPAMD.cpp b/clang/lib/Driver/ToolChains/HIPAMD.cpp
index ccb36a6c846c806..24f3db9001e1dd6 100644
--- a/clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ b/clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -119,8 +119,16 @@ void AMDGCN::Linker::constructLldCommand(Compilation &C, const JobAction &JA,
   auto &TC = getToolChain();
   auto &D = TC.getDriver();
   assert(!Inputs.empty() && "Must have at least one input.");
+  // Find the first filename InputInfo object.
+  auto Input = llvm::find_if(
+      Inputs, [](const InputInfo &II) -> bool { return II.isFilename(); });
+  if (Input == Inputs.end())
+    // For a very rare case, all of the inputs to the linker are
+    // InputArg. If that happens, just use the first InputInfo.
+    Input = Inputs.begin();
+
   bool IsThinLTO = D.getLTOMode(/*IsOffload=*/true) == LTOK_Thin;
-  addLTOOptions(TC, Args, LldArgs, Output, Inputs[0], IsThinLTO);
+  addLTOOptions(TC, Args, LldArgs, Output, *Input, IsThinLTO);
 
   // Extract all the -m options
   std::vector<llvm::StringRef> Features;
diff --git a/clang/lib/Driver/ToolChains/MinGW.cpp b/clang/lib/Driver/ToolChains/MinGW.cpp
index 5d7f8675daf8d28..066566eb02e4983 100644
--- a/clang/lib/Driver/ToolChains/MinGW.cpp
+++ b/clang/lib/Driver/ToolChains/MinGW.cpp
@@ -243,7 +243,15 @@ void tools::MinGW::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   if (D.isUsingLTO()) {
     assert(!Inputs.empty() && "Must have at least one input.");
-    addLTOOptions(TC, Args, CmdArgs, Output, Inputs[0],
+    // Find the first filename InputInfo object.
+    auto Input = llvm::find_if(
+        Inputs, [](const InputInfo &II) -> bool { return II.isFilename(); });
+    if (Input == Inputs.end())
+      // For a very rare case, all of the inputs to the linker are
+      // InputArg. If that happens, just use the first InputInfo.
+      Input = Inputs.begin();
+
+    addLTOOptions(TC, Args, CmdArgs, Output, *Input,
                   D.getLTOMode() == LTOK_Thin);
   }
 

[jhuber6]

Why couldn't you have put this logic in addLTOOptions? Seems like it's copy-pasted verbatim at every site now.

AMD should handle very similarly to Linux here. They both compile down to LLVM-IR and get sent to ld.lld.

[brad0]

Why couldn't you have put this logic in addLTOOptions? Seems like it's copy-pasted verbatim at every site now.

AMD should handle very similarly to Linux here. They both compile down to LLVM-IR and get sent to ld.lld.

It's not my area, but I was thinking that would make more sense. I am just trying to ensure consistency with the various targets.

[brad0]

@MaskRay

[MaskRay]

At this point, it does seem that changing addLTOOption to accept Inputs instead of Input will eliminate duplication and ensure consistency among targets.

[yxsamliu]

I think we should change signature of addLTOOptions. Instead of accepting a single InputInfo, it should accept InputInfoList. Then these ConstructJob members will pass Inputs to addLTOOptions, and addLTOOptions will pick the first file from it. This should be able to avoid repeated code.

[brad0]

@MaskRay Any response to what yxsamliu said?

[MaskRay]

We should change addLTSOption and let it perform:

    auto Input = llvm::find_if(
        Inputs, [](const InputInfo &II) -> bool { return II.isFilename(); });
    if (Input == Inputs.end())
      // For a very rare case, all of the inputs to the linker are
      // InputArg. If that happens, just use the first InputInfo.
      Input = Inputs.begin();

[arsenm]

Last comments requested changes

[jhuber6]

Looks like a nice cleanup