This is an archive of the discontinued LLVM Phabricator instance.

clang: Add triples support for MIPS r6
ClosedPublic

Authored by wzssyqa on Aug 16 2018, 10:20 AM.

Details

Summary

Debian uses different triples for MIPS r6 and paths.

These new triples include:

mipsisa32r6-linux-gnu
mipsisa32r6el-linux-gnu
mipsisa64r6-linux-gnuabi64
mipsisa64r6el-linux-gnuabi64
mipsisa64r6-linux-gnuabin32
mipsisa64r6el-linux-gnuabin32

This patch depends on: https://reviews.llvm.org/D50857

Diff Detail

Repository
rL LLVM

Event Timeline

wzssyqa created this revision.Aug 16 2018, 10:20 AM
wzssyqa retitled this revision from Add triples support for MIPS r6 to clang: Add triples support for MIPS r6.Aug 16 2018, 11:44 AM
wzssyqa edited the summary of this revision. (Show Details)

Could you add test cases to cover these changes?

lib/Driver/ToolChains/Arch/Mips.cpp
115 ↗(On Diff #161054)

It looks like this change is unrelated to introducing new target triples and can be made by a separate commit.

lib/Driver/ToolChains/Gnu.cpp
2068 ↗(On Diff #161054)

Ditto

2077 ↗(On Diff #161054)

Ditto

2085 ↗(On Diff #161054)

Ditto

2093 ↗(On Diff #161054)

Ditto

2437 ↗(On Diff #161054)

Are you sure that integrated LLVM assembler is ready to support N32 code generation? Anyway this change is for a separate patch.

lib/Driver/ToolChains/Linux.cpp
126 ↗(On Diff #161054)

Suppose there are both "/lib/mips64-linux-gnu" and "/lib/mipsisa64-linux-gnuabi64" paths on a disk and provided target triple is mipsisa64-linux-gnuabi64. Is it good that we return "mips64-linux-gnu" from this function anyway?

wzssyqa updated this revision to Diff 163221.Aug 29 2018, 3:53 PM

Could you please include more context to patches sent for review?
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

lib/Driver/ToolChains/Linux.cpp
46 ↗(On Diff #163221)

Maybe use bool IsR6 to make the following expressions shorter?

111 ↗(On Diff #163221)

clang-format these lines

wzssyqa updated this revision to Diff 163624.Sep 1 2018, 4:02 AM
wzssyqa updated this revision to Diff 166219.Sep 19 2018, 9:13 PM

Could you please update the patch against the current trunk?

wzssyqa added a subscriber: rsmith.Sep 27 2018, 9:13 AM

I updated N32 patch for clang.
Simon Atanasyan via Phabricator <reviews@reviews.llvm.org>
于2018年9月27日周四 下午8:23写道:

atanasyan added a comment.

Could you please update the patch against the current trunk?

Repository:

rC Clang

https://reviews.llvm.org/D50850

wzssyqa updated this revision to Diff 167419.Sep 27 2018, 5:55 PM

Fine. Now this patch has modifications for LLVM (not Clang) and all these changes were applied at rL343185 already. Could you attach an actual patch brings R6 support to the Clang driver?

This is really for Clang. I guess you mean that compiler-rt directory also need to be patched.

lib/Driver/ToolChains/Arch/Mips.cpp
115 ↗(On Diff #161054)

These lines code about N32, is quite close tied with r6 staffs, as they shared lots.

Is it ok to update the descriptions?

lib/Driver/ToolChains/Gnu.cpp
2093 ↗(On Diff #161054)

As a question: why MIPSTriples here?
the above mips64 lines don't include any EL Triples.

2437 ↗(On Diff #161054)

I didn't have so many test, while helloworld programs really works.

lib/Driver/ToolChains/Linux.cpp
126 ↗(On Diff #161054)

No, return `mips64-linux-gnu' is not good, I keep it just for to making sure I don't change the behavior of clang with my patch.

In fact, mips64-linux-gnu in gcc is N32, and Debian never use this triple, we use

mips*64*-linux-gnuabin32

So, on Debian, mips64-linux-gnu should never appear.

wzssyqa updated this revision to Diff 167630.Sep 29 2018, 6:46 PM

This is really for Clang. I guess you mean that compiler-rt directory also need to be patched.

If you take a look at the previous version of this patch https://reviews.llvm.org/D50850?id=167419, you see that it changed LLVM files. Probably you attached another diff.

As to compiler-rt - the Phabricator replace R6 symbols by the https://reviews.llvm.org/source/compiler-rt/ links. So my statement was "Could you attach an actual patch brings R6 support to the Clang driver?".

lib/Driver/ToolChains/Gnu.cpp
2093 ↗(On Diff #161054)

I do not remember exact answer, although I might be an author if this code. Maybe it handle some complicated directories tree from multi-arch toolchains. Are all tests passed if you remove this line?

2437 ↗(On Diff #161054)

You created a patch that teaches the Clang driver to understand (pass arguments to backend, find includes and libraries etc) N32 ABI better. Now I can compile "hello world". And after that you decided that LLVM backend does not have any MIPS N32 ABI related problems. I think we can enable the integrated assembler when a) it's possible to recursively build Clang with N32 ABI, b) all tests from LLVM test suite (https://llvm.org/docs/TestSuiteGuide.html) are passed in N32 ABI mode, c) we check that LLVM produces correct DWARF for N32.

Could you please rebase this patch against the trunk?

lib/Driver/ToolChains/Linux.cpp
717 ↗(On Diff #167630)

If we drop mipsisa64r6-linux-gnu, mipsisa64r6el-linux-gnu triples in the getMultiarchTriple function, why do we need these triple here?

This is really for Clang. I guess you mean that compiler-rt directory also need to be patched.

If you take a look at the previous version of this patch https://reviews.llvm.org/D50850?id=167419, you see that it changed LLVM files. Probably you attached another diff.

You are right. I uploaded the patch for llvm here by mistake.
And soon, I recognized, and then upload the right one.

As to compiler-rt - the Phabricator replace R6 symbols by the https://reviews.llvm.org/source/compiler-rt/ links. So my statement was "Could you attach an actual patch brings R6 support to the Clang driver?".

lib/Driver/ToolChains/Linux.cpp
126 ↗(On Diff #161054)

mips64-linux-gnu is removed and replaced by MipsCpu + "-linux-" + Mips64Abi

wzssyqa updated this revision to Diff 169810.Oct 16 2018, 5:07 AM
atanasyan accepted this revision.Oct 16 2018, 7:31 AM

LGTM. Thanks for the patch.

This revision is now accepted and ready to land.Oct 16 2018, 7:31 AM
This revision was automatically updated to reflect the committed changes.