Page MenuHomePhabricator

[RISCV] Default to ilp32d/lp64d in RISC-V Linux
ClosedPublic

Authored by rogfer01 on Aug 1 2019, 11:35 PM.

Details

Summary

When running clang as a native compiler in RISC-V Linux the flag -mabi=ilp32d / -mabi=lp64d is always mandatory. This change makes it the default there.

Diff Detail

Repository
rL LLVM

Event Timeline

rogfer01 created this revision.Aug 1 2019, 11:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 11:35 PM
asb added a comment.Aug 1 2019, 11:57 PM

Thank Roger. While we're doing this, I think it would make sense to default to ilp32d on 32-bit Linux? I know the glibc support etc is less mature than for RV64, but it seems the sensible thing to do.

lenary added inline comments.Aug 2 2019, 2:21 AM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
385 ↗(On Diff #212972)

Please may you turn this into a set of if-statements? Nesting ternary operators is a recipe for confusion.

rogfer01 updated this revision to Diff 213812.Aug 7 2019, 1:41 AM
rogfer01 retitled this revision from [RISCV] Default to lp64d in 64-bit RISC-V Linux to [RISCV] Default to ilp32d/lp64d in RISC-V Linux.
rogfer01 edited the summary of this revision. (Show Details)

ChangeLog:

  • Make ilp32d also the default in 32-bit RISC-V Linux
  • Do not use nested conditional expressions
rogfer01 marked an inline comment as done.Aug 7 2019, 1:42 AM

Thanks @asb @lenary for the review!

I understand that, after this change, we will also want to make -march=rv{32,64}gc the default in Linux as well. Otherwise there will be an ABI mismatch with the default -march=rv{32.64}i in a default invocation.

Does this make sense?

asb added a comment.Aug 8 2019, 4:05 AM

Thanks @asb @lenary for the review!

I understand that, after this change, we will also want to make -march=rv{32,64}gc the default in Linux as well. Otherwise there will be an ABI mismatch with the default -march=rv{32.64}i in a default invocation.

Does this make sense?

Yes, that makes sense to me.

luismarques added inline comments.Aug 15 2019, 8:47 AM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
386 ↗(On Diff #213812)

When I compile a bare metal GNU toolchain (using https://github.com/riscv/riscv-gnu-toolchain, reports GCC 8.3.0) I seem to get lp64d by default. Should we not match that behaviour?

$ cat test.c
float foo() { return 42.0; }
$ riscv64-unknown-elf-gcc -O2 -S test.c
$ cat test.s
(...)
foo:
        lui     a5,%hi(.LC0)
        flw     fa0,%lo(.LC0)(a5)
(...)
luismarques added inline comments.Aug 15 2019, 8:52 AM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
386 ↗(On Diff #213812)

To clarify, that's a toolchain configured with --enable-multilib.

lenary accepted this revision.Sep 9 2019, 7:41 AM

I think my feeling is that this patch can land and we can change the default abi for baremetal targets in a follow-up patch.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
386 ↗(On Diff #213812)

I'm confused by this @luismarques

If I do riscv64-unknown-elf-gcc -c test.c followed by riscv64-unknown-elf-objdump -f test.o, the flags displayed are 0x00000011, which does not include ELF::EF_RISCV_FLOAT_ABI_DOUBLE (0x4), and so suggests gcc is using -mabi=lp64 on baremetal elf targets.

That said, riscv64-unknown-elf-gcc -### -c test.c shows it's invoking all subtools with -march=rv64imafdc and -mabi-lp64d. This is still with --enable-multilib, using riscv64-unknown-elf-gcc (GCC) 8.3.0 and GNU objdump (GNU Binutils) 2.32.

I tried to look at where a default was being set in the gcc repo, and the only thing I can see is that the rv64imafdc/lp64d is the last combination to be generated in the multilib configuration, so they may not have explicitly chosen it as a default. I'm not sure.

This revision is now accepted and ready to land.Sep 9 2019, 7:41 AM

Thanks for the review @lenary @luismarques

We can indeed look at what defaults we want for baremetal in a later change.

I plan to commit this shortly.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 1:01 AM