currently only support the set of multilibs same to riscv-gnu-toolchain.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
1536 | It could be "riscv32-unknown-elf" other than "riscv64-unknown-elf". | |
clang/lib/Driver/ToolChains/RISCVToolchain.cpp | ||
136 | This change seems like unrelated to multi-lib, could you split this change into new patch and add a test for that? | |
139 | This part will conflict when apply patch, you might generate patch with https://reviews.llvm.org/D67409, could you rebase the patch with current trunk? |
Please can you rebase these changes? Something has changed in RISCVToolchain.cpp and they are failing to build.
Q: Is there a plan to support multilib aliases (MULTILIB_REUSE)? I'm happy for this to be in a follow-up patch, would just like to know the plan.
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
1518 | I cannot see march=rv64i/mabi=lp64 in riscv-gcc t-elf-multilib (the given sha is pulled from .gitmodules in riscv-gcc-toolchain. |
@lenary Sorry, I don't have the plan to support MULTILIB_REUSE now.
But if it's necessary to support it, I can do it later.
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
1518 | fixed! thanks! |
I finally got my system well setup enough to check this patch with my risc-v toolchains. It looks good, I'm happy for this to land.
There's no requirement to support MULTILIB_REUSE yet. It might be nice to add support in a follow-up patch, if you want.
Sorry for approving, and then requesting changes. I've been investigating issues with this patch.
When I try to use -print-multi-lib (a clang option that is very under-documented, but I think it's there to match GCC's option), this often doesn't always print out available multilibs, which is confusing.
- clang --gcc-toolchain=<..> --target=riscv32-unknown-elf -print-multi-lib does print out available multilibs (as gcc would)
- clang --gcc-toolchain=<..> --target=riscv64-unknown-elf -print-multi-lib does not print out anything.
- clang --gcc-toolchain=<..> --target=riscv64-unknown-elf -march=rv64gc -mabi=lp64d -print-multi-lib does not print anything.
- clang --gcc-toolchain=<..> --target=riscv64-unknown-elf -march=rv64imafdc -mabi=lp64d -print-multi-lib does print out available multilibs (as gcc would).
In all cases, I'm using a crosstool-ng configured toolchain for riscv64-unknown-elf.
I get the correct output for 2. if I change the line noted in the line comment below. Note that choosing a reasonable default here has to match other defaults chosen in clang.
I think the reason that 3. is not working is because of MULTILIB_REUSE. I do not think that blocks this patch, as I've said, this patch does not need to include MULTILIB_REUSE support.
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
1546 | I think this line is the issue: where someone doesn't specify -march, you choose a default -march that does not appear in the list of multilib arches in RISCVMultilibSet above. I think this issue probably didn't arise when you had rv64i/lp64 in the list, but given that's not in the riscv-gcc t-elf-multilib, so this code should choose a new default. I don't know how this new default will affect defaults chosen in other parts of the clang codebase. |
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
1546 | oh it's why I added rv64i/lp64 in previous version, but it's wrong default. |
Ok, found a path forward for this patch. Notes inline:
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
1546 | The solution for this patch is to use the function riscv::getRISCVABI(const ArgList &Args, const llvm::Triple &Triple) from clang/lib/Driver/ToolChains/Arch/RISCV.h. Then when I update the logic there to match gcc's logic, the right thing will happen. I will submit a patch to have this function match the gcc logic today. It took me a few hours but I found the default logic for GCC. |
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
1546 | Sorry, I'm wrong. I'm adding a riscv::getRISCVArch(...) to that file, which will solve this issue, sorry for the confusion. Will add this patch as dependent on that one when it's ready. |
@lenary
You patch is very useful to look up the default march, thanks!
But there is some issue if we set the default rv32 march as rv32gc.
Because the default multilib does not include rv32gc/lp32d in riscv gnu toolchain, https://github.com/riscv/riscv-gcc/blob/ed3f6ec/gcc/config/riscv/t-elf-multilib#L19
so if user does not give the default march clang will not find the library and cause linking error.
Do you think this is a clang's problem? or maybe this is just user's problem so they should config multilib to support rv32gc/lip3d2d?
Oh, I'm seeing how these pieces (don't) fit together now. Sorry for missing that the defaults in D69383 don't quite line up with this patch.
What happens if a user requests a t-multilib-elf build of GCC, without specifying --with-arch= or --with-abi=, then they get a default march/mabi combination that they don't have a multilib for, or do the definitions come from somewhere else? So I realise that the --with-arch= and --with-abi= options come from t-elf-multilib when gcc is built.
Looking closer at the t-elf-multilib file, there's not any ilp32d configuration, which seems like an oversight if people want to use that ABI on elf targets.
So, I guess there are three choices:
- We change the defaults in D69383 to match the elf multilibs that are built today. (i.e., one of the current multilib choices will become a default).
- We request a change in the t-elf-multilib MULTILIB_REQUIRED list to add rv32imafdc/ilp32d (and add an rv32gc/ilp32d alias to MULTILIB_REUSE) (This probably requires rebuilding existing multilib configurations, to compile this extra combination)
- We request a change in config.gcc (and I update my patch to reflect it) (This probably means having per-target defaults for linux vs elf, and ensuring these defaults are in the multilib file).
Part of the issue is it's not great to change defaults (we can probably only do it for clang 10, and never again), and it's not great to make people recompile their gcc toolchains either.
I have just updated D69383.
It now defaults to rv{XLEN}imac on ELF, rather than rv{XLEN}gc. This is to help this multilib issue.
In the future, we want to implement MULTILIB_REUSE properly. Hopefully before Clang 10.0.0 is released.
Do rebase this patch now that D69383 has landed, and check that it is still working correctly. If so, you can land this patch.
Re-applied with test fix in https://reviews.llvm.org/rG4fccd383d571865321b4723b81c3042d2c15fd80
I cannot see march=rv64i/mabi=lp64 in riscv-gcc t-elf-multilib (the given sha is pulled from .gitmodules in riscv-gcc-toolchain.