You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 Cranelift test_run command doesn't print skipped tests #1558)
compile and run the test using the default host TargetIsa
I propose we change the behavior to:
check if the default host TargetIsais 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
}
}
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_backendin atest runfiletest but theSingleFunctionCompilerdoesn'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:
SingleFunctionCompiler) is the same as the one specified by thetargetdirective; if not, skip the test (though in the future this should print some sort ofIGNORED [filename]so the user knows what happened, see Cranelifttest_runcommand doesn't print skipped tests #1558)TargetIsaI propose we change the behavior to:
TargetIsais compatible with the one specified by thetargetdirective; if not, skip the test, etc.targetdirective'sTargetIsa--this will use any special flags assigned by the user, e.g.use_new_backendTo get this "is compatible with" behavior, I propose we add a new
Flags::matchesfunction to the sharedFlagsand to each ISA-specificFlags(this likely has to be done as generated code ingen_settings.rsso it only needs to be done once). Then, we expose this function asTargetIsa::matchesand 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::matcheswork? I think it needs to iterate over the field descriptors and compare them both to the default value and to the other value. Remember thatmatchesonly goes one way: ifamatchesbit does not necessarily mean thatbmatchesa. Though it passes some simple tests, I'm not 100% confident that the following is correct so I would appreciate feedback: