This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add initial RISC-V target and driver support
ClosedPublic

Authored by asb on Nov 13 2017, 8:57 AM.

Diff Detail

Event Timeline

asb created this revision.Nov 13 2017, 8:57 AM
johnrusso added inline comments.Nov 13 2017, 1:02 PM
lib/Driver/ToolChains/RISCV.h
42

I wonder if we should construct the linker name based on the default triple value? "riscv64-unknown-elf-ld" or "riscv32-unknown-elf-ld" as this is likely what is in the user's PATH if they have installed the gnu tools. The linker name could be modified in ConstructJob if the triple is different from the default.

mgrang added a subscriber: mgrang.Nov 13 2017, 4:28 PM
mgrang added inline comments.
lib/Driver/ToolChains/RISCV.cpp
86

How about if our sysroot is linux (as opposed to elf)? There won't be any libgloss.a, right? Also there won't be a crt0.o (instead there will be crt1.o).

lib/Driver/ToolChains/RISCV.h
42

I agree with @johnrusso . I always have errors: "riscv32-ld not found" and as a workaround I end up creating symlink pointing to the actual <triple>-ld.

apazos added inline comments.Nov 13 2017, 4:59 PM
lib/Driver/ToolChains/RISCV.cpp
86

Supporting linux target is desirable early on because most of us will be using Qemu for running tests.

asb added inline comments.Nov 14 2017, 6:15 AM
lib/Driver/ToolChains/RISCV.cpp
86

Linux targets are not currently supported, as they require the ilp32d or lp64d (hard double precision) ABI. The only fiddly part is actually in the Clang frontend, handling structs composed of two reals or one integer + one real. This is obviously high up on the todo list.

@asb I cherry-picked this patch and was able to compile a simple program for elf triple. By manually adding a few libs on the link line I was also able to make it link for linux triple.
Could you please respond to the comment about risv32-ld? Other than that this patch LGTM!

mgrang added inline comments.Nov 15 2017, 2:58 PM
test/Driver/riscv64-toolchain.c
1

I just saw that this test fails with the error:

error: backend data layout 'e-m:e-i64:64-n32:64-S128' does not match expected target description 'e-m:e-p:64:64-i64:64-i128:128-n64-S128'

asb updated this revision to Diff 123229.Nov 16 2017, 12:13 PM
asb marked an inline comment as done.

Consider this a WIP update. This is not yet ready for merging, but could still benefit from feedback.

This update adds support for RISC-V to the Linux toolchain driver, which includes support for multilib selection. Unfortunately I had to add a special case to lib/Driver/Toolchains/Linux.cpp for adding lib paths laid out according to the RISC-V multilib conventions (e.g. $SYSROOT/lib32/ilp32, $SYSROOT/usr/lib32/ilp32). I notice that Mips had to do something similar, and ended up defining the MipsLinux class.

@jroelofs: Did you have any views one way or the other about extending BareMetal to support RISC-V as well as ARM?

asb marked 4 inline comments as done.Nov 16 2017, 12:16 PM
asb added inline comments.
lib/Driver/ToolChains/RISCV.cpp
86

It seems I was mistaken - soft float linux targets with glibc are working fine, meaning there's no need to wait for hardfloat ABI lowering.

Note that Linux targets are handled in lib/Driver/ToolChains/Linux.cpp

lib/Driver/ToolChains/RISCV.h
42

Now updated so the bin dir from the given GCC install (passed by --gcc-toolchain) is added to the PATH, and the default search logic will look for an ld named after after the target triple.

mgrang added inline comments.Nov 16 2017, 12:19 PM
test/Driver/riscv64-toolchain.c
1

Pushed a fix for this: https://reviews.llvm.org/D40145

mgrang added inline comments.Nov 16 2017, 1:09 PM
lib/Driver/ToolChains/Gnu.cpp
1779

I suppose we need riscv64-unknown-elf here too?

lib/Driver/ToolChains/Linux.cpp
228 โ†—(On Diff #123229)

This seems incorrect. RHS should be:

Arch == llvm::Triple::riscv32 || Arch == llvm::Triple::riscv64;

Can you push this as a patch to review/commit instead of RFC? It has received a lot of comments/corrections already and I think it is getting in a shape we can merge.

lib/Driver/ToolChains/RISCV.cpp
86

I meant user code compiled with target=riscv32-unknown-linux march=rv32imafdc and mabi=ilp32. This would run fine on qemu.

test/Preprocessor/init.c
9991

Shouldn't we just check for the target specific defines in these tests?

asb marked an inline comment as done.Nov 23 2017, 7:20 AM

Can you push this as a patch to review/commit instead of RFC? It has received a lot of comments/corrections already and I think it is getting in a shape we can merge.

I might move the baremetal support to separate patch and discuss that on the mailing list. I'll remove the [RFC] tag in the next update, adding tests.

test/Preprocessor/init.c
9991

Many of the other tests seem to test a similarly large set of defines, plus I think you _do_ want to check the majority of these (especially the ones related to C types).

mgrang added inline comments.Nov 30 2017, 8:12 AM
lib/Driver/ToolChains/Arch/RISCV.cpp
47 โ†—(On Diff #123229)

Can you also add a case for pushing back +c here?

asb updated this revision to Diff 126404.Dec 11 2017, 10:58 AM
asb marked 3 inline comments as done.
asb retitled this revision from [RISCV][RFC] Add initial RISC-V target and driver support to [RISCV] Add initial RISC-V target and driver support.
asb edited the summary of this revision. (Show Details)

Patch updates:

  • Address review comments
  • (u)int64_type is long int on RV64 rather than long long int
  • Remove driver for bare metal RV32 target (to be posted in a subsequent patch and maybe discussed on cfe-dev).

From my perspective, the only deficiency in this patch is now the tests (which I'll add to this review ASAP). Let me know if you see other remaining problems.

asb updated this revision to Diff 126769.Dec 13 2017, 9:09 AM
asb edited the summary of this revision. (Show Details)

Update to add test cases based on the multilib Linux SDK produced by https://github.com/riscv/riscv-gnu-toolchain/.

asb updated this revision to Diff 126771.Dec 13 2017, 9:14 AM

Apologies, the last version had a few lines of debug code left in.

I should say that this is to the best of my knowledge ready to merge (i.e. there are no outstanding flagged issues). Particularly now that the majority of the RV32I codegen patches are merged in LLVM, it would be really helpful to get this merged in to clang asap in order to make larger scale testing of the LLVM backend possible without out-of-tree patches.

efriedma added a subscriber: efriedma.

Adding some reviewers to hopefully find someone comfortable reviewing the Linux multilib bits. (Not sure I'm adding the right people; this stuff hasn't been touched for a while, as far as I can tell.)

The rest LGTM.

apazos accepted this revision.Jan 8 2018, 12:28 PM

Please merge this patch, it looks in good shape. This patch is required for any RISCV build.

This revision is now accepted and ready to land.Jan 8 2018, 12:28 PM
This revision was automatically updated to reflect the committed changes.

This has broken our toolchain build, the log is here: https://logs.chromium.org/v/?s=fuchsia%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8957686819564148864%2F%2B%2Fsteps%2Fcheck_clang%2F0%2Fstdout. The failure is:

******************** TEST 'Clang :: Driver/riscv32-toolchain.c' FAILED ********************
Script:
--
/b/s/w/ir/tmp/rt/clangM7g4Ie/llvm_build_dir/tools/clang/stage2-bins/bin/clang /b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c -### -no-canonical-prefixes -target riscv32 2>&1 | /b/s/w/ir/tmp/rt/clangM7g4Ie/llvm_build_dir/tools/clang/stage2-bins/bin/FileCheck -check-prefix=CC1 /b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c
/b/s/w/ir/tmp/rt/clangM7g4Ie/llvm_build_dir/tools/clang/stage2-bins/bin/clang /b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c -### -no-canonical-prefixes    -target riscv32-linux-unknown-elf    --gcc-toolchain=/b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/Inputs/multilib_riscv_linux_sdk    --sysroot=/b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1    | /b/s/w/ir/tmp/rt/clangM7g4Ie/llvm_build_dir/tools/clang/stage2-bins/bin/FileCheck -check-prefix=CC1-RV32-LINUX-ILP32 /b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c
/b/s/w/ir/tmp/rt/clangM7g4Ie/llvm_build_dir/tools/clang/stage2-bins/bin/clang /b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c -### -no-canonical-prefixes    -target riscv32-linux-unknown-elf -march=rv32imafd -mabi=ilp32d    --gcc-toolchain=/b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/Inputs/multilib_riscv_linux_sdk    --sysroot=/b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/Inputs/multilib_riscv_linux_sdk/sysroot 2>&1    | /b/s/w/ir/tmp/rt/clangM7g4Ie/llvm_build_dir/tools/clang/stage2-bins/bin/FileCheck -check-prefix=CC1-RV32-LINUX-ILP32D /b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c
/b/s/w/ir/tmp/rt/clangM7g4Ie/llvm_build_dir/tools/clang/stage2-bins/bin/clang -target riscv32 /b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c -emit-llvm -S -o - | /b/s/w/ir/tmp/rt/clangM7g4Ie/llvm_build_dir/tools/clang/stage2-bins/bin/FileCheck /b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c
--
Exit Code: 1
Command Output (stderr):
--
/b/s/w/ir/kitchen-workdir/llvm-project/clang/test/Driver/riscv32-toolchain.c:13:26: error: expected string not found in input
// CC1-RV32-LINUX-ILP32: "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../../../riscv64-unknown-linux-gnu/bin{{/|\\\\}}ld"
                         ^
<stdin>:1:1: note: scanning from here
Fuchsia clang version 7.0.0 (https://fuchsia.googlesource.com/a/third_party/clang 6d538af698b9b96bc18bfbe62173dca1c103aaca) (https://fuchsia.googlesource.com/a/third_party/llvm 0b5677cbf87647986caf9fa818adb7f8c355cf1f) (based on LLVM 7.0.0svn)
^
--

I think the problem is that in our toolchain, we configure LLD as the default linker which is why that check is failing.

asb added a comment.Jan 13 2018, 1:26 AM

Hi Petr, thanks for the report. This should be addressed in rL322435.