Stream: git-wasmtime

Topic: wasmtime / Issue #1891 Test specific flags in Cranelift f...

Wasmtime GitHub notifications bot (Jun 17 2020 at 15:52):

abrown opened Issue #1891:

In #1880 @bnjbvr brought up an issue with how Cranelift's filetests use (or don't use) ISA-specific flags. He was trying to use target x86_64 use_new_backend in a test run filetest but the SingleFunctionCompiler doesn't know anything about ISA-specific Flags, only shared Flags. (Perhaps these two sets of flags should be merged in some way, but what I will propose next doesn't require that).

The current behavior is:

• check if the default host architecture (used by SingleFunctionCompiler) is the same as the one specified by the target directive; if not, skip the test (though in the future this should print some sort of IGNORED [filename] so the user knows what happened, see #1558)
• compile and run the test using the default host TargetIsa

I propose we change the behavior to:

• check if the default host TargetIsa _is compatible with_ the one specified by the target directive; if not, skip the test, etc.
• compile and run the test using the target directive's TargetIsa--this will use any special flags assigned by the user, e.g. use_new_backend

To get this "_is compatible with_" behavior, I propose we add a new Flags::matches function to the shared Flags and to each ISA-specific Flags (this likely has to be done as generated code in gen_settings.rs so it only needs to be done once). Then, we expose this function as TargetIsa::matches and implement it in each ISA as self.flags.matches(&other.flags) && self.isa_flags.matches(&other.isa_flags). This way we can compare TargetIsa's for compatibility.

How does Flags::matches work? I think it needs to iterate over the field descriptors and compare them both to the default value and to the other value. Remember that matches only goes one way: if a matches b it does not necessarily mean that b matches a. Though it passes some simple tests, I'm not 100% confident that the following is correct so I would appreciate feedback:

fn get_bit(byte: u8, bit: u8) -> bool {
    let mask = 1 << bit;
    (byte & mask) != 0
}

impl Flags {
    fn matches(&self, other: &Self) -> bool {
        let shared_default = settings::Flags::new(settings::builder());
        let default = Flags::new(&shared_default, builder());

        // Check each detail until we see presets.
        let mut byte_offset = 0;
        for d in &DESCRIPTORS {
            byte_offset = d.offset as usize;
            match d.detail {
                detail::Detail::Bool { bit } => {
                    let self_bit = get_bit(self.bytes[byte_offset], bit);
                    let default_bit = get_bit(default.bytes[byte_offset], bit);
                    let other_bit = get_bit(other.bytes[byte_offset], bit);
                    if self_bit != default_bit && self_bit != other_bit {
                        return false;
                    }
                }
                detail::Detail::Num | detail::Detail::Enum { .. } => {
                    let self_byte = self.bytes[byte_offset];
                    let default_byte = default.bytes[byte_offset];
                    let other_byte = other.bytes[byte_offset];
                    if self_byte != default_byte && self_byte != other_byte {
                        return false;
                    }
                }
                detail::Detail::Preset => break,
            }
        }

        // Then check each preset bit.
        for byte_offset in byte_offset + 1..self.bytes.len() {
            for bit in 0..8 {
                let self_bit = get_bit(self.bytes[byte_offset], bit);
                let default_bit = get_bit(default.bytes[byte_offset], bit);
                let other_bit = get_bit(other.bytes[byte_offset], bit);
                if self_bit != default_bit && self_bit != other_bit {
                    return false;
                }
            }
        }

        true
    }
}

Wasmtime GitHub notifications bot (Jun 17 2020 at 16:05):

abrown commented on Issue #1891:

Other thoughts:

• the function should probably be named has_compatible_flags or is_compatible_with or something like that
• @bnjbvr had issues getting Parser to set the right CallConv--the host's--for the functions it parsed; I think what I described above avoids this issue because we only would be comparing flags and since https://github.com/bytecodealliance/cranelift/pull/942 Parser is already setting the calling convention to the host's default, see https://github.com/bytecodealliance/wasmtime/blob/master/cranelift/reader/src/parser.rs#L96-L101.

Wasmtime GitHub notifications bot (Jun 17 2020 at 16:05):

abrown edited a comment on Issue #1891:

Other thoughts:

• the function should probably be named has_compatible_flags or is_compatible_with or something like that
• @bnjbvr had issues getting Parser to set the right CallConv--the host's--for the functions it parsed; I think what I described above avoids this issue because we only would be comparing flags and https://github.com/bytecodealliance/cranelift/pull/942 Parser is already setting the calling convention to the host's default, see https://github.com/bytecodealliance/wasmtime/blob/master/cranelift/reader/src/parser.rs#L96-L101.

Wasmtime GitHub notifications bot (Jun 18 2020 at 10:14):

bnjbvr commented on Issue #1891:

Thanks for opening an issue! I think the above is a correct description of the problems I've ran into.

• agreed with is_compatible_with which is a better name than matches (although that's the one i used in our zulip discussions :))
• the issue with CallConv is two-fold: the SingleFunctionCompiler will check that the calling convention defined by the ISA defined the one used by the function (= the one defined by the parser). But if we use the TargetIsa defined by the target directive (from the parser), since the target directive doesn't know about the OS that's used for testing, it'll use the SystemV calling convention by default. This caused issues on Windows; my partial workaround was to try to set the Triple's missing fields with the Triple's host fields when they were missing during parser, but it was not pretty at all. Perhaps we could just override the CallConv field of the Function's signature and get away with it; I haven't tried this (there might be other places where the initial CallConv setting leaked, e.g. libcalls).

Regarding the Flags::matches implementation, at a skim it seems correct. I wonder if checking for the setting is set to the default value is correct; there's no way to distinguish between "the value has been manually set to use the default" vs "the value hasn't been set at all" (and we'd probably want to do the full if check only in the latter case). We probably don't need to check for presets either, since they're just sets of other settings that could be manually defined without using presets (for instance, one can set add to the target directive the use_sse_41 and use_popcnt flags and get the same set of features that nehalem enables, without explicitly using the nehalem preset).

(Bonus points if you can get a way to have x86 32-bits tests running on x86 64-bits hosts :-).)

Wasmtime GitHub notifications bot (Jun 29 2020 at 15:53):

bnjbvr labeled Issue #1891:

In #1880 @bnjbvr brought up an issue with how Cranelift's filetests use (or don't use) ISA-specific flags. He was trying to use target x86_64 use_new_backend in a test run filetest but the SingleFunctionCompiler doesn't know anything about ISA-specific Flags, only shared Flags. (Perhaps these two sets of flags should be merged in some way, but what I will propose next doesn't require that).

The current behavior is:

• check if the default host architecture (used by SingleFunctionCompiler) is the same as the one specified by the target directive; if not, skip the test (though in the future this should print some sort of IGNORED [filename] so the user knows what happened, see #1558)
• compile and run the test using the default host TargetIsa

I propose we change the behavior to:

• check if the default host TargetIsa _is compatible with_ the one specified by the target directive; if not, skip the test, etc.
• compile and run the test using the target directive's TargetIsa--this will use any special flags assigned by the user, e.g. use_new_backend

To get this "_is compatible with_" behavior, I propose we add a new Flags::matches function to the shared Flags and to each ISA-specific Flags (this likely has to be done as generated code in gen_settings.rs so it only needs to be done once). Then, we expose this function as TargetIsa::matches and implement it in each ISA as self.flags.matches(&other.flags) && self.isa_flags.matches(&other.isa_flags). This way we can compare TargetIsa's for compatibility.

How does Flags::matches work? I think it needs to iterate over the field descriptors and compare them both to the default value and to the other value. Remember that matches only goes one way: if a matches b it does not necessarily mean that b matches a. Though it passes some simple tests, I'm not 100% confident that the following is correct so I would appreciate feedback:

fn get_bit(byte: u8, bit: u8) -> bool {
    let mask = 1 << bit;
    (byte & mask) != 0
}

impl Flags {
    fn matches(&self, other: &Self) -> bool {
        let shared_default = settings::Flags::new(settings::builder());
        let default = Flags::new(&shared_default, builder());

        // Check each detail until we see presets.
        let mut byte_offset = 0;
        for d in &DESCRIPTORS {
            byte_offset = d.offset as usize;
            match d.detail {
                detail::Detail::Bool { bit } => {
                    let self_bit = get_bit(self.bytes[byte_offset], bit);
                    let default_bit = get_bit(default.bytes[byte_offset], bit);
                    let other_bit = get_bit(other.bytes[byte_offset], bit);
                    if self_bit != default_bit && self_bit != other_bit {
                        return false;
                    }
                }
                detail::Detail::Num | detail::Detail::Enum { .. } => {
                    let self_byte = self.bytes[byte_offset];
                    let default_byte = default.bytes[byte_offset];
                    let other_byte = other.bytes[byte_offset];
                    if self_byte != default_byte && self_byte != other_byte {
                        return false;
                    }
                }
                detail::Detail::Preset => break,
            }
        }

        // Then check each preset bit.
        for byte_offset in byte_offset + 1..self.bytes.len() {
            for bit in 0..8 {
                let self_bit = get_bit(self.bytes[byte_offset], bit);
                let default_bit = get_bit(default.bytes[byte_offset], bit);
                let other_bit = get_bit(other.bytes[byte_offset], bit);
                if self_bit != default_bit && self_bit != other_bit {
                    return false;
                }
            }
        }

        true
    }
}

Wasmtime GitHub notifications bot (Jun 29 2020 at 15:53):

github-actions[bot] commented on Issue #1891:

Subscribe to Label Action

cc @bnjbvr

<details>
This issue or pull request has been labeled: "cranelift"

Thus the following users have been cc'd because of the following labels:

• bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Last updated: Nov 22 2024 at 16:03 UTC