[PATCH] Support Sourcery CodeBench MIPS toolchain

[Rafael Ávila de Espíndola]

  Mostly nits. Sorry for taking so long to review this. Would you mind uploading an updated patch with the comments addressed. I will be much faster to review the next version.


================
Comment at: lib/Driver/ToolChains.cpp:1125
@@ -1125,1 +1124,3 @@
+    "mipsel-linux-android",
+    "mips-linux-gnu"
   };
----------------
mips-linux-gnu is used for both LE and BE?


================
Comment at: lib/Driver/ToolChains.cpp:1301
@@ -1269,6 +1300,3 @@
 
-static StringRef getTargetMultiarchSuffix(llvm::Triple::ArchType TargetArch,
-                                          const ArgList &Args) {
-  if (TargetArch == llvm::Triple::x86_64 ||
-      TargetArch == llvm::Triple::ppc64)
-    return "/64";
+static void getMipsTargetSuffix(SmallString<32> &Path,
+                                llvm::Triple::ArchType TargetArch,
----------------
s/get/append/ since this modifies the Path.

================
Comment at: lib/Driver/ToolChains.cpp:1304
@@ -1275,1 +1303,3 @@
+                                const ArgList &Args) {
+  Path.clear();
 
----------------
Why?

================
Comment at: lib/Driver/ToolChains.cpp:1319
@@ +1318,3 @@
+
+static bool getTargetMultiarchSuffix(SmallString<32> &Suffix,
+                                     StringRef Path,
----------------
s/get/append/

================
Comment at: lib/Driver/ToolChains.cpp:1326
@@ +1325,3 @@
+
+    if (TargetArch == llvm::Triple::mips64 ||
+        TargetArch == llvm::Triple::mips64el)
----------------
Cant you merge this to getMipsTargetSuffix?

================
Comment at: lib/Driver/ToolChains.cpp:1330
@@ +1329,3 @@
+
+    if (llvm::sys::fs::exists(Path + Suffix.str() + "/crtbegin.o"))
+      return true;
----------------
Just

return llvm::sys::fs::exists(Path + Suffix.str() + "/crtbegin.o";

no?

================
Comment at: lib/Driver/ToolChains.cpp:1339
@@ +1338,3 @@
+           TargetArch == llvm::Triple::mips64el)
+    Suffix = hasMipsN32ABIArg(Args) ? "/n32" : "/64";
+  else
----------------
Can you please merge this with the above so that the entire funciton looks like

getTargetMultiarchSuffix(...) {
​  if (isMipsArch(TargetArch)) {
     getMipsTargetSuffix(..);
    return ...;
 }
  // not mips case.
}

================
Comment at: lib/Driver/ToolChains.cpp:2172
@@ +2171,3 @@
+
+  if (IsMips && !SysRoot.empty() && D.SysRoot.empty())
+    ExtraOpts.push_back("--sysroot=" + SysRoot);
----------------
Checking both Sysroot and D.SysRoot is redundant given


	​std::string Linux::computeSysRoot(const ArgList &Args) const {
		​  if (!getDriver().SysRoot.empty())
		​    return getDriver().SysRoot;

no?


http://llvm-reviews.chandlerc.com/D644