[PATCH] D13340: Add support for the new mips-mti-linux toolchain.

[Simon Atanasyan via cfe-commits]

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