This is an archive of the discontinued LLVM Phabricator instance.

clang: fix MIPS/N32 triple and paths
ClosedPublic

Authored by wzssyqa on Aug 29 2018, 3:56 PM.

Details

Summary

Guess N32 ABI when no abi option is given based on llvm patch.
It now support mips64(el)-linux-gnuabin32 and mipsn32(el).

The include and library paths are also added based on
Debian/Gcc scheme.

Diff Detail

Repository
rC Clang

Event Timeline

wzssyqa created this revision.Aug 29 2018, 3:56 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/Arch/Mips.cpp
109

If target triple is mips-mti-linux-gnuabin32 the code above set (incorrectly) the ABIName to n64 and this statement will be false.

lib/Driver/ToolChains/Linux.cpp
47
  • Do you need MipsCpu variable?
  • Is it possible to use any lightweight type like StringRef for the Mips64Abi?
118

If a user has two toolchains installed into "/lib/mips64-linux-gnu" and "/lib/mips64-linux-gnuabin32", this code always selects mips64-linux-gnu even if N32 ABI is requested. Is it intended?

wzssyqa updated this revision to Diff 163622.Sep 1 2018, 2:41 AM

Remove unused MipsCpu.

wzssyqa updated this revision to Diff 166220.Sep 19 2018, 9:15 PM

remove mips64(el)-linux-gnu from path search.

atanasyan requested changes to this revision.Sep 20 2018, 4:39 AM

This patch fails the following test cases:

  • tools/clang/test/CodeGen/target-data.c
  • tools/clang/test/Driver/mips-cs.cpp
lib/Basic/Targets/Mips.h
75

Let's write all cases in a uniform manner:

if (Triple.isMIPS32())
  setABI("o32");
else if (Triple.getEnvironment() == llvm::Triple::GNUABIN32)
  setABI("n32");
else
  setABI("n64");
lib/Driver/ToolChains/Arch/Mips.cpp
112

What about the following combination of a command line arguments?

-target mips-mti-linux-gnuabin32 -mips64

In that case, ABIName is empty, Triple.getVendor() returns MipsTechnologies, CPUName is "mips64". So ABIName becomes "n64". And this new if statement doesn't work.

lib/Driver/ToolChains/Gnu.cpp
2431

Before this change we enable integrated assembler for mips64/mips64el architectures only when we are sure that target ABI is N64. The problem is that there are some bugs which do not allow the integrated assembler generates correct N32 code in all cases. After this change we enable integrated assembler for N32 ABI. I do not think it's a good idea now.

If we can pass command line arguments to this routine, it probably would be possible to detect N32 ABI by checking both GNUABIN32 environment and -mabi=n32 option. And disable integrated assembler for MIPS targets in that case only. But anyway this change is for another patch.

This revision now requires changes to proceed.Sep 20 2018, 4:39 AM
wzssyqa updated this revision to Diff 167340.Sep 27 2018, 9:12 AM
atanasyan added inline comments.Sep 28 2018, 7:16 AM
lib/Driver/ToolChains/Arch/Mips.cpp
88

Is possible to rewrite this piece of code (lines 85-114) as follows?

if (ABIName.empty() && (Triple.getEnvironment() == llvm::Triple::GNUABIN32))
  ABIName = "n32";

if (ABIName.empty() &&
    (Triple.getVendor() == llvm::Triple::MipsTechnologies ||
     Triple.getVendor() == llvm::Triple::ImaginationTechnologies)) {
  ABIName = llvm::StringSwitch<const char *>(CPUName)
                .Case("mips1", "o32")
                .Case("mips2", "o32")
                .Case("mips3", "n64")
                .Case("mips4", "n64")
                .Case("mips5", "n64")
                .Case("mips32", "o32")
                .Case("mips32r2", "o32")
                .Case("mips32r3", "o32")
                .Case("mips32r5", "o32")
                .Case("mips32r6", "o32")
                .Case("mips64", "n64")
                .Case("mips64r2", "n64")
                .Case("mips64r3", "n64")
                .Case("mips64r5", "n64")
                .Case("mips64r6", "n64")
                .Case("octeon", "n64")
                .Case("p5600", "o32")
                .Default("");
}
lib/Driver/ToolChains/Gnu.cpp
2087

Why do you remove BiarchTripleAliases.append(begin(MIPSTriples), end(MIPSTriples)); in that case only? Is it intended?

wzssyqa updated this revision to Diff 167631.Sep 29 2018, 6:50 PM
wzssyqa updated this revision to Diff 167632.Sep 29 2018, 7:00 PM

Please run test suite before sending a patch to review. After applying this patch the following tests failed:

  • test/CodeGen/target-data.c
  • test/Driver/mips-cs.cpp
wzssyqa updated this revision to Diff 168116.Oct 3 2018, 8:12 AM

ohhh. make check-all is needed, instead of make check

test/CodeGen/target-data.c

is due to duplicate line `MIPS-64EL: target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-n32:64-S128"'

test/Driver/mips-cs.cpp

is due to this test use the hardcode path `/mips-linux-gnu/', so mipsel and mips64el also need
    BiarchTripleAliases.append(begin(MIPSTriples), end(MIPSTriples));
although MIPSTriples should in the last order.
lib/Driver/ToolChains/Arch/Mips.cpp
88

OK. I didn't do like this because I am not very full confident about whether there are some case that it is set to N32 even -gnuabin32 is used.

109

as I know mips-mti-linux-gnuabin32 is never used, at least for gcc.
Should we allow it?

lib/Driver/ToolChains/Gnu.cpp
2087

ohhh, my fault. I should remove from mipsel also.

lib/Driver/ToolChains/Linux.cpp
47

oh, MipsCpu is not used here, while used in D50850.
In that patch. we need different CPU names:

mipsel vs mipsisa32r6el

etc. It is my fault to split the patches.

StringRef is not OK as, the return value of getMultiarchTriple is std::string.

118

Yes. It is intended.

I don't want my patch change the behavior of llvm/clang:

the previous of llvm/clang behavior is perfering /lib/mips64-linux-gnu.

And on Debian, /lib/mips64-linux-gnu should never exists, we use

/lib/mips64-linux-gnuabi64

and

/lib/mips64-linux-gnuabin32
118

If there is a SDK, which is using /lib/mips64-linux-gnu', and for Debian we use /lib/mips64-linux-gnuabi64':

there must at least one cannot work. So here, I choose to make sure the exists SDK working and leave the risk to Debian's official toolchain.
as the `/lib/mips64-linux-gnu' is previous than my patch by time.

By FHS and Debian's practice, cross toolchains should never be put under /lib directly, these manually installed software should exist in /usr/local or /opt.

I'm going to test current MIPS N32 ABI implementation. Maybe we are ready to enable integrated assembler for it. In that case both Generic_GCC::IsIntegratedAssemblerDefault() and MipsMCAsmInfo ctor can be simplified.

atanasyan accepted this revision.Oct 15 2018, 3:44 PM

LGTM. Thanks for the patch.

This revision is now accepted and ready to land.Oct 15 2018, 3:44 PM
This revision was automatically updated to reflect the committed changes.