[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
https://github.com/arsenm requested changes to this pull request. Last comments requested changes https://github.com/llvm/llvm-project/pull/74178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
MaskRay wrote:
We should change `addLTSOption` and let it perform:
```
auto Input = llvm::find_if(
Inputs, [](const InputInfo ) -> 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();
```
https://github.com/llvm/llvm-project/pull/74178
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
brad0 wrote: @MaskRay Any response to what [yxsamliu](https://github.com/yxsamliu) said? https://github.com/llvm/llvm-project/pull/74178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
yxsamliu wrote: 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. https://github.com/llvm/llvm-project/pull/74178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
MaskRay wrote: At this point, it does seem that changing `addLTOOption` to accept `Inputs` instead of `Input` will eliminate duplication and ensure consistency among targets. https://github.com/llvm/llvm-project/pull/74178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
brad0 wrote: @MaskRay https://github.com/llvm/llvm-project/pull/74178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
brad0 wrote: > 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. https://github.com/llvm/llvm-project/pull/74178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
https://github.com/jhuber6 commented: 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`. https://github.com/llvm/llvm-project/pull/74178 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Brad Smith (brad0)
Changes
Copying
https://github.com/llvm/llvm-project/commit/1881832994840baa6e42f908b8822ce4d15ab632
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
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 , const
JobAction ,
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 ) -> 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 ,
const JobAction ,
auto = getToolChain();
auto = 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 ) -> 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 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 ,
const JobAction ,
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 ) -> 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);
}
``
https://github.com/llvm/llvm-project/pull/74178
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Driver][LTO] Copy fix empty stats filename to AMDGPU, HIPAMD, MinGW (PR #74178)
https://github.com/brad0 created https://github.com/llvm/llvm-project/pull/74178
Copying
https://github.com/llvm/llvm-project/commit/1881832994840baa6e42f908b8822ce4d15ab632
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?
>From e475c3e4c3fdcb3fa869279ec65a2cf866b7d48e Mon Sep 17 00:00:00 2001
From: Brad Smith
Date: Sat, 11 Nov 2023 20:02:40 -0500
Subject: [PATCH] [Driver][LTO] Copy fix empty stats filename to AMDGPU,
HIPAMD, MinGW
---
clang/lib/Driver/ToolChains/AMDGPU.cpp | 15 ---
clang/lib/Driver/ToolChains/HIPAMD.cpp | 10 +-
clang/lib/Driver/ToolChains/MinGW.cpp | 10 +-
3 files changed, 30 insertions(+), 5 deletions(-)
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 , const
JobAction ,
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 ) -> 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 ,
const JobAction ,
auto = getToolChain();
auto = 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 ) -> 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 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 ,
const JobAction ,
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 ) -> 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);
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
