Page MenuHomePhabricator

[RISCV] support mutilib in baremetal environment
ClosedPublic

Authored by khchen on Sep 12 2019, 10:04 AM.

Diff Detail

Event Timeline

khchen created this revision.Sep 12 2019, 10:04 AM
lenary added a reviewer: asb.Sep 12 2019, 10:15 AM
khchen set the repository for this revision to rC Clang.Sep 12 2019, 5:27 PM
khchen added a project: Restricted Project.
kito-cheng added inline comments.Sep 17 2019, 3:29 PM
clang/lib/Driver/ToolChains/Gnu.cpp
1536

It could be "riscv32-unknown-elf" other than "riscv64-unknown-elf".

clang/lib/Driver/ToolChains/RISCVToolchain.cpp
135

This change seems like unrelated to multi-lib, could you split this change into new patch and add a test for that?

137–138

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?

khchen updated this revision to Diff 220686.Sep 18 2019, 8:56 AM
khchen marked an inline comment as done.
khchen updated this revision to Diff 220687.Sep 18 2019, 8:59 AM
khchen marked 3 inline comments as done.Sep 18 2019, 9:02 AM
khchen added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
1536

fixed

clang/lib/Driver/ToolChains/RISCVToolchain.cpp
135

Enabling multi-lib uses /Inputs/multilib_riscv_elf_sdk/riscv64-unknown-elf/bin/ld so we need to specify the emulation format.

137–138

fixed

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.

khchen updated this revision to Diff 225068.Oct 15 2019, 10:25 AM
khchen edited the summary of this revision. (Show Details)

@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.

khchen marked 2 inline comments as done.Oct 15 2019, 10:27 AM
khchen added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
1518

fixed! thanks!

lenary accepted this revision.Oct 21 2019, 9:38 AM

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.

This revision is now accepted and ready to land.Oct 21 2019, 9:38 AM
lenary requested changes to this revision.Oct 23 2019, 8:10 AM

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.

  1. clang --gcc-toolchain=<..> --target=riscv32-unknown-elf -print-multi-lib does print out available multilibs (as gcc would)
  2. clang --gcc-toolchain=<..> --target=riscv64-unknown-elf -print-multi-lib does not print out anything.
  3. clang --gcc-toolchain=<..> --target=riscv64-unknown-elf -march=rv64gc -mabi=lp64d -print-multi-lib does not print anything.
  4. 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.

This revision now requires changes to proceed.Oct 23 2019, 8:10 AM
khchen marked 2 inline comments as done.Oct 23 2019, 10:10 PM
khchen added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
1546

oh it's why I added rv64i/lp64 in previous version, but it's wrong default.
in riscv gcc if configure only with --enable-multilib, the default march/abi is rv64imafdc/lp64d
The option 1 is aligning to gcc, but the default abi is lp64 in clang,
maybe chosing rv64imac as default is less side effect? what do you think?

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.

lenary added inline comments.Oct 24 2019, 6:02 AM
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?

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:

  1. 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).
  2. 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)
  3. 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.

khchen updated this revision to Diff 227606.Nov 3 2019, 7:07 AM
khchen set the repository for this revision to rG LLVM Github Monorepo.Nov 3 2019, 7:16 AM
lenary accepted this revision.Nov 4 2019, 6:25 AM

LGTM. Requires the changes in D69383 to land before this can.

clang/lib/Driver/ToolChains/Gnu.cpp
1545

Nice, thanks!

This revision is now accepted and ready to land.Nov 4 2019, 6:25 AM

Do rebase this patch now that D69383 has landed, and check that it is still working correctly. If so, you can land this patch.

khchen updated this revision to Diff 229966.Mon, Nov 18, 8:23 PM

rebase and fix failed testcases

Closed by commit rGb6d7bbfa0043: [RISCV] Support mutilib in baremetal environment (authored by khchen, committed by Zakk Chen <zakk.chen@sifive.com>). · Explain WhyTue, Nov 19, 2:19 AM
This revision was automatically updated to reflect the committed changes.

Re-applied with typo fix in rGdf876a026981.