wzssyqa added a comment.
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.
================
Comment at: lib/Driver/ToolChains/Arch/Mips.cpp:109
+ if (ABIName.empty() && (Triple.getEnvironment() == llvm::Triple::GNUABIN32))
+ ABIName = "n32";
----------------
atanasyan wrote:
> If target triple is mips-mti-linux-gnuabin32 the code above set (incorrectly)
> the `ABIName` to `n64` and this statement will be `false`.
as I know mips-mti-linux-gnuabin32 is never used, at least for gcc.
Should we allow it?
================
Comment at: lib/Driver/ToolChains/Arch/Mips.cpp:85
if (ABIName.empty() &&
(Triple.getVendor() == llvm::Triple::MipsTechnologies ||
----------------
atanasyan wrote:
> 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("");
> }
> ```
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.
================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2082
BiarchTripleAliases.append(begin(MIPSELTriples), end(MIPSELTriples));
- BiarchTripleAliases.append(begin(MIPSTriples), end(MIPSTriples));
+ BiarchLibDirs.append(begin(MIPSN32ELLibDirs), end(MIPSN32ELLibDirs));
+ BiarchTripleAliases.append(begin(MIPSN32ELTriples), end(MIPSN32ELTriples));
----------------
atanasyan wrote:
> Why do you remove `BiarchTripleAliases.append(begin(MIPSTriples),
> end(MIPSTriples));` in that case only? Is it intended?
ohhh, my fault. I should remove from mipsel also.
================
Comment at: lib/Driver/ToolChains/Linux.cpp:47
bool IsAndroid = TargetTriple.isAndroid();
+ std::string MipsCpu = "", Mips64Abi = "gnuabi64";
+ if (TargetEnvironment == llvm::Triple::GNUABIN32)
----------------
atanasyan wrote:
> - Do you need `MipsCpu` variable?
> - Is it possible to use any lightweight type like `StringRef` for the
> `Mips64Abi`?
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.
================
Comment at: lib/Driver/ToolChains/Linux.cpp:118
case llvm::Triple::mips64:
if (D.getVFS().exists(SysRoot + "/lib/mips64-linux-gnu"))
return "mips64-linux-gnu";
----------------
atanasyan wrote:
> 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?
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
================
Comment at: lib/Driver/ToolChains/Linux.cpp:118
case llvm::Triple::mips64:
if (D.getVFS().exists(SysRoot + "/lib/mips64-linux-gnu"))
return "mips64-linux-gnu";
----------------
wzssyqa wrote:
> atanasyan wrote:
> > 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?
> 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
>
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.
https://reviews.llvm.org/D51464
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
