diff mbox series

[v2,2/3] scripts: generate_rust_target: enable building on RISC-V

Message ID 20240223-employee-pessimism-03ba0b58db6b@spud
State New
Headers
Series RISC-V: enable rust |

Commit Message

Conor Dooley Feb. 23, 2024, 1:38 p.m. UTC 
  From: Miguel Ojeda <ojeda@kernel.org>

Add the required bits from rust-for-linux to enable generating a RISC-V
target for rust. The script, written by Miguel, was originally a
config file contributed by Gary.

Co-developed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 scripts/generate_rust_target.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Miguel Ojeda Feb. 23, 2024, 2:39 p.m. UTC | #1 
On Fri, Feb 23, 2024 at 2:38 PM Conor Dooley <conor@kernel.org> wrote:
>
> Add the required bits from rust-for-linux to enable generating a RISC-V
> target for rust. The script, written by Miguel, was originally a
> config file contributed by Gary.

Thanks for this Connor!

arm64 is sending these for 6.9:

    https://git.kernel.org/arm64/c/f82811e22b48
    https://git.kernel.org/arm64/c/724a75ac9542

So it would be nice to see if it may be already possible to enable it
via a builtin target + flags instead of the custom target, e.g. arm64
does:

    KBUILD_RUSTFLAGS += --target=aarch64-unknown-none -Ctarget-feature="-neon"

and so on.

If it does not work, it would be good to know what would be needed for
RISC-V and put it into the unstable features / wanted features list
for Rust.

Either way, it is not a blocker (although you will need a rebase after
arm64 lands to use the `target.json` in the right places).

Cheers,
Miguel
Conor Dooley Feb. 27, 2024, 10:16 a.m. UTC | #2 
On Fri, Feb 23, 2024 at 03:39:47PM +0100, Miguel Ojeda wrote:

> Conor Dooley <conor@kernel.org> wrote:

> Thanks for this Connor!




> arm64 is sending these for 6.9:
> 
>     https://git.kernel.org/arm64/c/f82811e22b48
>     https://git.kernel.org/arm64/c/724a75ac9542
> 
> So it would be nice to see if it may be already possible to enable it
> via a builtin target + flags instead of the custom target, e.g. arm64
> does:
> 
>     KBUILD_RUSTFLAGS += --target=aarch64-unknown-none -Ctarget-feature="-neon"
> 
> and so on.
> 
> If it does not work, it would be good to know what would be needed for
> RISC-V and put it into the unstable features / wanted features list
> for Rust.

Sure, I'll take a look.

> Either way, it is not a blocker (although you will need a rebase after
> arm64 lands to use the `target.json` in the right places).

Nah, I think that is silly. Either this goes in as-is, and there's
fixup done by Linus, or the thing should be converted to match arm64,
assuming that that is possible.

Cheers,
Conor.
Miguel Ojeda Feb. 27, 2024, 10:46 a.m. UTC | #3 
On Tue, Feb 27, 2024 at 11:17 AM Conor Dooley
<conor.dooley@microchip.com> wrote:
>
> Sure, I'll take a look.

Thanks!

> Nah, I think that is silly. Either this goes in as-is, and there's
> fixup done by Linus, or the thing should be converted to match arm64,
> assuming that that is possible.

Ah, so you are going for 6.9 too? I can give the series a try on my
side in that case. When do you plan to apply them?

Cheers,
Miguel
Conor Dooley Feb. 27, 2024, 11:04 a.m. UTC | #4 
On Tue, Feb 27, 2024 at 11:46:42AM +0100, Miguel Ojeda wrote:
> On Tue, Feb 27, 2024 at 11:17 AM Conor Dooley
> <conor.dooley@microchip.com> wrote:
> >
> > Sure, I'll take a look.
> 
> Thanks!
> 
> > Nah, I think that is silly. Either this goes in as-is, and there's
> > fixup done by Linus, or the thing should be converted to match arm64,
> > assuming that that is possible.
> 
> Ah, so you are going for 6.9 too? I can give the series a try on my
> side in that case. When do you plan to apply them?

If they're to be applied, it would be Palmer, not I. And if history is
any guide, it could be into the merge window. My point though was more
that either this was acceptable for v6.9 or would be v6.10 material
with the same mechanism as arm64. Rebasing after v6.9-rc1 but not
adapting to that way of doing things is what seemed silly to me, since
if a resend is required then the other improvements should be carried
out at the same time.

Cheers,
Conor.
Miguel Ojeda Feb. 27, 2024, 12:12 p.m. UTC | #5 
On Tue, Feb 27, 2024 at 12:05 PM Conor Dooley
<conor.dooley@microchip.com> wrote:
>
> My point though was more
> that either this was acceptable for v6.9 or would be v6.10 material
> with the same mechanism as arm64. Rebasing after v6.9-rc1 but not
> adapting to that way of doing things is what seemed silly to me, since
> if a resend is required then the other improvements should be carried
> out at the same time.

If avoiding the `target.json` is possible, definitely.

I didn't want to assume it is, though -- e.g. the native integer
widths you have is 64 but the built-in targets use 32:64 (perhaps
there is a way to tweak it with an LLVM param via `-Cllvm-args`, but I
don't see any obvious way from a quick look; `opt` does have it,
though).

(That is why we supported `target.json`, since it gives the most
freedom in the beginning.)

Cheers,
Miguel
Conor Dooley Feb. 27, 2024, 12:36 p.m. UTC | #6 
On Tue, Feb 27, 2024 at 01:12:51PM +0100, Miguel Ojeda wrote:
> On Tue, Feb 27, 2024 at 12:05 PM Conor Dooley
> <conor.dooley@microchip.com> wrote:
> >
> > My point though was more
> > that either this was acceptable for v6.9 or would be v6.10 material
> > with the same mechanism as arm64. Rebasing after v6.9-rc1 but not
> > adapting to that way of doing things is what seemed silly to me, since
> > if a resend is required then the other improvements should be carried
> > out at the same time.
> 
> If avoiding the `target.json` is possible, definitely.
> 
> I didn't want to assume it is, though -- e.g. the native integer
> widths you have is 64 but the built-in targets use 32:64 (perhaps
> there is a way to tweak it with an LLVM param via `-Cllvm-args`, but I
> don't see any obvious way from a quick look; `opt` does have it,
> though).

Looking closer at those targets, all of them enable compressed
instructors, but we support hardware that does not support them.
I think that means we are stuck with the custom targets.

I could badger Palmer to pick this up tomorrow, provided you're okay
with the gist of this series.

Cheers,
Conor.
Miguel Ojeda Feb. 27, 2024, 2:47 p.m. UTC | #7 
On Tue, Feb 27, 2024 at 1:37 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> Looking closer at those targets, all of them enable compressed
> instructors, but we support hardware that does not support them.
> I think that means we are stuck with the custom targets.

Did you try `-Ctarget-feature=-c`? i.e. as far as I know, you can
disable target features even if they are enabled from the built-in.

It seems to work from a quick try (in userspace), e.g.

0000000000000000 <f>:
       0: 9b 05 05 00      sext.w    a1, a0
       4: 13 05 a0 02      li    a0, 0x2a
       8: 63 84 05 00      beqz    a1, 0x10 <f+0x10>
       c: 13 05 b0 07      li    a0, 0x7b
      10: 67 80 00 00      ret

vs.

0000000000000000 <f>:
       0: 9b 05 05 00      sext.w    a1, a0
       4: 13 05 a0 02      li    a0, 0x2a
       8: 99 c1            beqz    a1, 0xe <f+0xe>
       a: 13 05 b0 07      li    a0, 0x7b
       e: 82 80            ret

Cheers,
Miguel
Conor Dooley Feb. 27, 2024, 5:48 p.m. UTC | #8 
On Tue, Feb 27, 2024 at 03:47:29PM +0100, Miguel Ojeda wrote:

> Did you try `-Ctarget-feature=-c`? i.e. as far as I know, you can
> disable target features even if they are enabled from the built-in.

No, I didn't actually try anything. Between trying to test the kcfi
stuff on arm64 and the other work I was doing today I did not have a
chance to actually play with that yet. It comes down to you though I
suppose - would you rather have generate_rust_target enable the
compressed instructions depending on the config option or have the
Makefile disable it if compressed instructions are not enabled and use
a builtin target?
Miguel Ojeda Feb. 27, 2024, 6:11 p.m. UTC | #9 
On Tue, Feb 27, 2024 at 6:48 PM Conor Dooley <conor@kernel.org> wrote:
>
> No, I didn't actually try anything. Between trying to test the kcfi
> stuff on arm64 and the other work I was doing today I did not have a
> chance to actually play with that yet.

Apologies, I didn't mean it that way -- I was just wondering if it
didn't work. No rush on my side (in fact, I thought this was for 6.10
anyway).

> It comes down to you though I
> suppose - would you rather have generate_rust_target enable the
> compressed instructions depending on the config option or have the
> Makefile disable it if compressed instructions are not enabled and use
> a builtin target?

So the custom target support is there for flexibility purposes: we
were told is that since the target spec is too tied to LLVM, it is
unlikely to get stabilized (at least as it is), and thus they
preferred that we ask for whatever flags in `rustc` would be needed to
tweak things in an existing builtin target (or add new built-in
targets if needed).

Thus, when a new architecture is added, the question is whether one
can already use the flags approach or not.

For instance, to disable the compressed instructions, from what I can
tell, the flag I mentioned seems to work, so that is fine. However,
for something like tweaking the data layout for `n64` instead of
`n32:64`, I am not aware of a way to do so via a flag (but I see newer
LLVM uses `n32:64`, so that may be the better one anyway:
https://godbolt.org/z/Eh4cfdeMr).

So it all depends on whether you are happy with what the flags
approach already give you.

I hope that clarifies a bit!

Cheers,
Miguel
Conor Dooley Feb. 27, 2024, 6:24 p.m. UTC | #10 
On Tue, Feb 27, 2024 at 07:11:17PM +0100, Miguel Ojeda wrote:
> On Tue, Feb 27, 2024 at 6:48 PM Conor Dooley <conor@kernel.org> wrote:

> For instance, to disable the compressed instructions, from what I can
> tell, the flag I mentioned seems to work, so that is fine.

To me, using flags is a good fit given that's what we do elsewhere,
and there won't be a mix of stuff that is done conditionally in a
script and conditionally in a Makefile.

> However,
> for something like tweaking the data layout for `n64` instead of
> `n32:64`, I am not aware of a way to do so via a flag (but I see newer
> LLVM uses `n32:64`, so that may be the better one anyway:
> https://godbolt.org/z/Eh4cfdeMr).

Yeah, I had looked at the blame for the targets earlier today and
noticed that it had been changed. Sadly rustc's commit is lacking
any justification whatsoever for the change, so I was not going to
really comment on that until I had looked.

> So it all depends on whether you are happy with what the flags
> approach already give you.
> 
> I hope that clarifies a bit!

Ye, thanks. I'll give it a go when I have a bit of time this week.
diff mbox series

Patch

diff --git a/scripts/generate_rust_target.rs b/scripts/generate_rust_target.rs
index 416c6b89e806..942ddca57ee4 100644
--- a/scripts/generate_rust_target.rs
+++ b/scripts/generate_rust_target.rs
@@ -171,6 +171,22 @@  fn main() {
         ts.push("llvm-target", "loongarch64-linux-gnusf");
         ts.push("llvm-abiname", "lp64s");
         ts.push("target-pointer-width", "64");
+    } else if cfg.has("RISCV") {
+        if cfg.has("64BIT") {
+            ts.push("arch", "riscv64");
+            ts.push("data-layout", "e-m:e-p:64:64-i64:64-i128:128-n64-S128");
+            ts.push("llvm-target", "riscv64-linux-gnu");
+            ts.push("target-pointer-width", "64");
+        } else {
+            panic!("32-bit RISC-V is an unsupported architecture");
+        }
+        ts.push("code-model", "medium");
+        ts.push("disable-redzone", true);
+        let mut features = "+m,+a".to_string();
+        if cfg.has("RISCV_ISA_C") {
+            features += ",+c";
+        }
+        ts.push("features", features);
     } else {
         panic!("Unsupported architecture");
     }