Stream: git-wasmtime

Topic: wasmtime / PR #6192 cranelift-native: Detect RISC-V exten...

Wasmtime GitHub notifications bot (Apr 10 2023 at 16:41):

afonso360 opened PR #6192 from afonso360:riscv-cpuinfo to bytecodealliance:main:

:wave: Hey,

This PR adds a second method of extension detection to cranelift-native for RISC-V. Namely we can now read the ISA string in /proc/cpuinfo to query extenstions.

The motivation for this is that HWCAP does not expose bits for multi letter extensions (i.e. Zba, Zbb, Zbs). These are all extension that we already have support for in cranelift, but since we can't detect them, they don't get tested with runtests. (So far I've been testing them by force enabling them).

With this method we can now detect them. The second issue is that the current QEMU version does not do /proc/cpuinfo emulation, so we need to patch it. The patch in this PR has already been accepted upstream and is queued for QEMU 8.1. The version included here is slightly different since I had to backport it to 7.2.0.

Additionally as part of the SIMD work for RISC-V, I discovered that QEMU has a fixed set of bits for HWCAP, and so does not correctly expose the enabled extensions (namely vectors), so I can't run runtests with that either.

I've also tested introducing a bug in one of the Zba lowerings and running CI on that just to doublecheck that all fo this works. (logs)

Wasmtime GitHub notifications bot (Apr 10 2023 at 16:41):

afonso360 requested jameysharp for a review on PR #6192.

Wasmtime GitHub notifications bot (Apr 10 2023 at 16:41):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #6192.

Wasmtime GitHub notifications bot (Apr 10 2023 at 16:41):

afonso360 requested wasmtime-default-reviewers for a review on PR #6192.

Wasmtime GitHub notifications bot (Apr 10 2023 at 16:42):

afonso360 edited PR #6192:

:wave: Hey,

This PR adds a second method of extension detection to cranelift-native for RISC-V. Namely we can now read the ISA string in /proc/cpuinfo to query extenstions.

The motivation for this is that HWCAP does not expose bits for multi letter extensions (i.e. Zba, Zbb, Zbs). These are all extension that we already have support for in cranelift, but since we can't detect them, they don't get tested with runtests. (So far I've been testing them by force enabling them).

Additionally as part of the SIMD work for RISC-V, I discovered that QEMU has a fixed set of bits for HWCAP, and so does not correctly expose the enabled extensions (namely vectors), so I can't run runtests with that either.

With this method we can now detect them. The second issue is that the current QEMU version does not do /proc/cpuinfo emulation, so we need to patch it. The patch in this PR has already been accepted upstream and is queued for QEMU 8.1. The version included here is slightly different since I had to backport it to 7.2.0.

I've also tested introducing a bug in one of the Zba lowerings and running CI on that just to doublecheck that all fo this works. (logs)

Wasmtime GitHub notifications bot (Apr 10 2023 at 20:37):

afonso360 edited PR #6192:

:wave: Hey,

This PR adds a second method of extension detection to cranelift-native for RISC-V. Namely we can now read the ISA string in /proc/cpuinfo to query extenstions.

The motivation for this is that HWCAP does not expose bits for multi letter extensions (i.e. Zba, Zbb, Zbs). These are all extension that we already have support for in cranelift, but since we can't detect them, they don't get tested with runtests. (So far I've been testing them by force enabling them).

Additionally as part of the SIMD work for RISC-V, I discovered that QEMU has a fixed set of bits for HWCAP, and so does not correctly expose the enabled extensions (namely vectors), so I can't run runtests with that either.

With this method we can now detect them. The second issue is that the current QEMU version does not do /proc/cpuinfo emulation, so we need to patch it. The patch in this PR has already been accepted upstream and is queued for QEMU 8.1. The version included here is slightly different since I had to backport it to 7.2.0.

I've also tested introducing a bug in one of the Zba lowerings and running CI on that just to doublecheck that all of this works. (logs)

Wasmtime GitHub notifications bot (Apr 10 2023 at 22:19):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Apr 10 2023 at 22:19):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Apr 10 2023 at 22:19):

jameysharp created PR review comment:

I'm always nervous about using starts_with in this way, in case somebody adds another key where the name begins with "isa".

I'd prefer to use l.split_once(':'). If it returns Some((k, v)) and k.trim_end() == "isa", then we want to parse v.trim().

Also, usually I prefer to use iterator chains anywhere I can, but in this case I think dealing with the possibility of I/O errors is obscuring the logic. Something like this gets nested more deeply than I'd prefer but I think it's better overall:

for line in BufReader::new(file).lines() {
    let line = line.map_err(|_| "failed to read /proc/cpuinfo")?;
    if let Some((k, v)) = line.split_once(':') {
        if k.trim_end() == "isa" {
            ... isa_string_extensions(v.trim()) ...
            return Ok(());
        }
    }
}
Err("no isa line found in /proc/cpuinfo")

As a further refinement, I think it's reasonable to return Ok when the isa line is not found, in which case you could return std::io::Result<()> and remove all the map_err calls.

Wasmtime GitHub notifications bot (Apr 10 2023 at 22:19):

jameysharp created PR review comment:

Returning an iterator can make sense if it lets you avoid heap allocations, but in this case it requires allocating a Vec for each group. So I lean toward just accumulating one Vec for all the results and returning that. This isn't performance-critical, anyway.

I think I'd prefer to verify that the rv64 part is the first one, which we can't cleanly do in an iterator chain, but can do by calling Iterator::next directly.

Finally, I fiddled around with different ways to get an iterator over single-character string slices. I like s.matches(|_| true) better than having to filter out empty strings from str::split.

 fn isa_string_extensions(isa: &str) -> Vec<&str> {
    let mut parts = isa.split('_');
    let mut extensions = Vec::new();
    // The first entry has the form `rv64imafdcvh`, we need to skip the architecture ("rv64").
    // Each of the letters after the cpu architecture is an extension, so return them
    // individually.
    if let Some(letters) = parts.next().unwrap().strip_prefix("rv64") {
        extensions.extend(letters.matches(|_| true));
        extensions.extend(parts);
    }
    extensions
}

Wasmtime GitHub notifications bot (Apr 11 2023 at 09:14):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2023 at 09:14):

afonso360 created PR review comment:

Yeah, I'm ok with that version as well. I tried to get a version that would just neatly return an iterator without allocating but couldn't get it. I left it in in the hopes that someone in the future would just see that and improve it.

Wasmtime GitHub notifications bot (Apr 11 2023 at 09:27):

afonso360 updated PR #6192.

Wasmtime GitHub notifications bot (Apr 11 2023 at 09:31):

afonso360 created PR review comment:

What do you think about this? It avoids all of the nesting, unnecessarily verbose error handling and displays the line parsing logic a bit better.

Wasmtime GitHub notifications bot (Apr 11 2023 at 09:31):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2023 at 16:48):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Apr 11 2023 at 16:48):

jameysharp created PR review comment:

Works for me, thank you!

Wasmtime GitHub notifications bot (Apr 11 2023 at 18:12):

afonso360 merged PR #6192.

Last updated: Nov 22 2024 at 16:03 UTC