[PATCH] D13340: Add support for the new mips-mti-linux toolchain.
Simon Atanasyan via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 1 06:31:07 PDT 2015
atanasyan added inline comments.
================
Comment at: include/clang/Driver/ToolChain.h:92
@@ -91,2 +91,3 @@
MultilibSet Multilibs;
+ Multilib SelectedMultilib;
----------------
This field is used by the `MipsToolChain` class only. If so, move it to that class.
================
Comment at: include/clang/Driver/ToolChain.h:147
@@ -145,1 +146,3 @@
+ const Multilib &getSelectedMultilib() const { return SelectedMultilib; }
+
----------------
Do you need public access to this member function?
================
Comment at: lib/Driver/Driver.cpp:2127
@@ +2126,3 @@
+ // Allow the discovery of tools prefixed with LLVM's default target triple.
+ std::string LLVMDefaultTargetTriple = llvm::sys::getDefaultTargetTriple();
+ if (LLVMDefaultTargetTriple != DefaultTargetTriple)
----------------
Just curious when is DefaultTargetTriple not equal to LLVMDefaultTargetTriple?
================
Comment at: lib/Driver/Driver.cpp:2225
@@ -2219,1 +2224,3 @@
TC = new toolchains::HexagonToolChain(*this, Target, Args);
+ else if (Target.getVendor() == llvm::Triple::MipsTechnologies)
+ TC = new toolchains::MipsToolChain(*this, Target, Args);
----------------
The `mips-mti-linux-gnu` triple is used by Codescape toolchain too. After this change if user provides `-target mips-mti-linux-gnu` command line option, the `MipsToolChain` will be used. As far as I understand you have to put `GCCInstallation.isValid()` checking to the `MipsToolChain` class methods to allow working with both GNU and non-GNU toolchains. IMHO it does not make the code clear. Maybe use the `MipsToolChain` class for the non-GNU toolchain only.
================
Comment at: lib/Driver/ToolChains.cpp:2210
@@ -2172,2 +2209,3 @@
const Generic_GCC::GCCVersion &V = GCCInstallation.getVersion();
+ const llvm::Triple &TT = getTriple();
bool UseInitArrayDefault =
----------------
Let's make this change by a separate commit to reduce number of unrelated changes.
================
Comment at: lib/Driver/ToolChains.cpp:2231
@@ +2230,3 @@
+ // If we did find a valid GCC installation, we don't have anything else to do.
+ if (GCCInstallation.isValid())
+ return;
----------------
When is GCCInstallation invalid in case of using this toolchain?
================
Comment at: lib/Driver/ToolChains.cpp:2243
@@ +2242,3 @@
+ tools::mips::getMipsCPUAndABI(Args, Triple, CPUName, ABIName);
+ LibSuffix = llvm::StringSwitch<std::string>(ABIName)
+ .Case("o32", "")
----------------
Similar code exists in the `getLinuxDynamicLinker` routine. Let's factor out it into the separate function say `tools::mips::getMipsABILibSuffix`.
================
Comment at: lib/Driver/Tools.cpp:8115
@@ -8110,2 +8114,3 @@
const llvm::Triple::ArchType Arch = ToolChain.getArch();
+ const llvm::Triple &TT = ToolChain.getTriple();
----------------
Let's make this change by a separate commit to reduce number of unrelated changes.
http://reviews.llvm.org/D13340
More information about the cfe-commits
mailing list