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 atest run
filetest but theSingleFunctionCompiler
doesn't know anything about ISA-specificFlags
, only sharedFlags
. (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 thetarget
directive; if not, skip the test (though in the future this should print some sort ofIGNORED [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 thetarget
directive; if not, skip the test, etc.- compile and run the test using the
target
directive'sTargetIsa
--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 sharedFlags
and to each ISA-specificFlags
(this likely has to be done as generated code ingen_settings.rs
so it only needs to be done once). Then, we expose this function asTargetIsa::matches
and implement it in each ISA asself.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 thatmatches
only goes one way: ifa
matchesb
it does not necessarily mean thatb
matchesa
. 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 } }
abrown commented on Issue #1891:
Other thoughts:
- the function should probably be named
has_compatible_flags
oris_compatible_with
or something like that- @bnjbvr had issues getting
Parser
to set the rightCallConv
--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/942Parser
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.
abrown edited a comment on Issue #1891:
Other thoughts:
- the function should probably be named
has_compatible_flags
oris_compatible_with
or something like that- @bnjbvr had issues getting
Parser
to set the rightCallConv
--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/942Parser
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.
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 thanmatches
(although that's the one i used in our zulip discussions :))- the issue with
CallConv
is two-fold: theSingleFunctionCompiler
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 theTargetIsa
defined by thetarget
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 theCallConv
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 fullif
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 thetarget
directive theuse_sse_41
anduse_popcnt
flags and get the same set of features thatnehalem
enables, without explicitly using thenehalem
preset).(Bonus points if you can get a way to have x86 32-bits tests running on x86 64-bits hosts :-).)
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 atest run
filetest but theSingleFunctionCompiler
doesn't know anything about ISA-specificFlags
, only sharedFlags
. (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 thetarget
directive; if not, skip the test (though in the future this should print some sort ofIGNORED [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 thetarget
directive; if not, skip the test, etc.- compile and run the test using the
target
directive'sTargetIsa
--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 sharedFlags
and to each ISA-specificFlags
(this likely has to be done as generated code ingen_settings.rs
so it only needs to be done once). Then, we expose this function asTargetIsa::matches
and implement it in each ISA asself.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 thatmatches
only goes one way: ifa
matchesb
it does not necessarily mean thatb
matchesa
. 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 } }
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: Dec 23 2024 at 12:05 UTC