Page MenuHomePhabricator

Add support for the new mips-mti-linux toolchain.
ClosedPublic

Authored by vkalintiris on Oct 1 2015, 5:13 AM.

Details

Summary

This new toolchain uses primarily LLVM-based tools, eg. compiler-rt, lld,
libcxx, etc. Because of this, it doesn't require neither an existing GCC
installation nor a GNU environment. Ideally, in a follow-up patch we
would like to add a new --{llvm|clang}-toolchain option (similar to
--gcc-toolchain) in order to allow the use of this toolchain with
independent Clang builds. For the time being, we use the --sysroot
option just to test the correctness of the paths generated by the
driver.

Diff Detail

Event Timeline

vkalintiris updated this revision to Diff 36227.Oct 1 2015, 5:13 AM
vkalintiris retitled this revision from to Add support for the new mips-mti-linux toolchain..
vkalintiris updated this object.
vkalintiris added reviewers: atanasyan, dsanders, rsmith.
vkalintiris added a subscriber: cfe-commits.
atanasyan added inline comments.Oct 1 2015, 6:31 AM
include/clang/Driver/ToolChain.h
92

This field is used by the MipsToolChain class only. If so, move it to that class.

147

Do you need public access to this member function?

lib/Driver/Driver.cpp
2127

Just curious when is DefaultTargetTriple not equal to LLVMDefaultTargetTriple?

2225

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.

lib/Driver/ToolChains.cpp
2210

Let's make this change by a separate commit to reduce number of unrelated changes.

2231

When is GCCInstallation invalid in case of using this toolchain?

2243

Similar code exists in the getLinuxDynamicLinker routine. Let's factor out it into the separate function say tools::mips::getMipsABILibSuffix.

lib/Driver/Tools.cpp
8115

Let's make this change by a separate commit to reduce number of unrelated changes.

vkalintiris updated this revision to Diff 36257.Oct 1 2015, 9:28 AM
vkalintiris marked 8 inline comments as done.

Thanks. The review comments are addressed in this update.

include/clang/Driver/ToolChain.h
147

I discarded this member function after the move of SelectedMultilib to the MipsToolChain class.

lib/Driver/Driver.cpp
2127

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.

2225

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.

lib/Driver/ToolChains.cpp
2231

I changed that, according to your comments earlier. Now we instantiate this class only for triples that don't have an environment.

atanasyan added inline comments.Oct 2 2015, 2:13 AM
lib/Driver/Driver.cpp
2225

Now I try to redesign Codescape toolchain support in the Clang driver. I consider to use a separate toolchain class like your MipsToolChain and I name it CodeScapeMtiToolChain. If I be able to join support for both MIT and IMG toolchains in the single class, I will rename it to CodeScapeToolChain.

Will this non-GNU toolchain have a personal name like CodeScape? If not I am okay with MipsNonGNUToolChain.

vkalintiris added inline comments.Oct 2 2015, 2:57 AM
lib/Driver/Driver.cpp
2225

I thought about this and I don't believe that MipsNonGNUToolChain is a very good name. The main reason is that the last component of the entries under the sysroot, contain the name of the C library (or empty for GLIBC). We could easily have a toolchain with mips-r2-hard{,-uClibc} installed for mips-mti-linux.

What are your thoughts about the names: MipsGCCToolChain, Mips{Clang,LLVM}ToolChain, or CodescapeGCCToolChain and CodescapeLLVMToolChain?

Personally, I'd prefer the first pair of names. The reason is that with this choice we will be consistent in the naming of our classes. Also, it's clear that these are MIPS TCs and we have to consider that the Codescape name could change in the future (improbable but not impossible).

The Mips{GCC,Clang/LLVM}Toolchain name would specify whether we are going to use the files generated from a GCC or Clang installation. This way we could keep the existing functionality of the Linux class for older toolchains that will be deprecated/unused over time.

atanasyan added inline comments.Oct 2 2015, 4:00 AM
lib/Driver/Driver.cpp
2225

It sounds reasonable. Let's use MipsGCCToolChain and MipsLLVMToolChain.

vkalintiris updated this revision to Diff 36344.Oct 2 2015, 5:04 AM
  1. s/MipsToolChain/MipsLLVMToolChain/
  2. Change to the new getCompilerRT() return type
  3. clang-format
atanasyan accepted this revision.Oct 2 2015, 6:42 AM
atanasyan edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 2 2015, 6:42 AM
This revision was automatically updated to reflect the committed changes.
vkalintiris marked an inline comment as done.