| Message ID | 20240223-employee-pessimism-03ba0b58db6b@spud (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | RISC-V: enable rust | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
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
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.
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
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.
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
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.
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
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?
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
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 --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"); }