-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW #74178
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Brad Smith (brad0) ChangesCopying 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:
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);
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
At this point, it does seem that changing |
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. |
We should change
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last comments requested 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?