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

[Vasileios Kalintiris via cfe-commits]

vkalintiris added inline comments.

================
Comment at: include/clang/Driver/ToolChain.h:147
@@ -145,1 +146,3 @@
 
+  const Multilib &getSelectedMultilib() const { return SelectedMultilib; }
+
----------------
atanasyan wrote:
> Do you need public access to this member function?
I discarded this member function after the move of `SelectedMultilib` to the `MipsToolChain` class.

================
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)
----------------
atanasyan wrote:
> Just curious when is DefaultTargetTriple not equal to LLVMDefaultTargetTriple?
In this case, `DefaultTargetTriple` is the triple specified with `--target=`. LLVMDefaultTargetTriple is the value specified with the CMake variable `-DLLVM_DEFAULT_TARGET_TRIPLE=` during configuration time. For example, using `--target=mips64el-mti-linux` will search for files prefixed with either `mips64el-mti-linux-{as,ld}` and `mips-mti-linux-{as,ld}` in our toolchain where we specify `-DLLVM_DEFAULT_TARGET_TRIPLE=mips-mti-linux.`

================
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);
----------------
atanasyan wrote:
> 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.
Shall we change the `MipsToolChain` class name to reflect these changes? Maybe `MipsNonGNUToolChain`? It would be strange to keep the same name, because most of the MIPS toolchains wouldn't be instantiated by `MipsToolChain`.

================
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;
----------------
atanasyan wrote:
> When is GCCInstallation invalid in case of using this toolchain?
I changed that, according to your comments earlier. Now we instantiate this class only for triples that don't have an environment.


http://reviews.llvm.org/D13340