abrown opened PR #1916 from fix-1891
to master
:
This change fixes the issues described in #1891:
- it makes the test-running infrastructure aware of what filetests will not be able to run on the current host machine by means of new implementations of
TargetIsa::is_compatible_with
andFlags::is_compatible_with
(generated)- it fixes the issue @bnjbvr described where the
TargetIsa
created byParser
is unaware of what host it is running on: fortest run
filetests, this is fixed with a use ofTriple::host()
I now feel like
is_compatible_with
(likematches
) is the wrong wording for that function; it should express that one configuration subsumes another so that it is clear that the opposite may not be true. I'm open to suggestions on that. I also likely need to adjust the ordering of commits at some point.
abrown requested bnjbvr for a review on PR #1916.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
fmtln!(fmt, "Detail::Preset => {},");
This should not short-circuit.
abrown submitted PR Review.
abrown created PR Review Comment:
I ended up omitting the
self.shared_flags.is_compatible_with(&other.shared_flags)
here because here I'm saying "self.isa_flags is more specific than other.isa_flags" (e.g. the host machine has more ISA flags set than the filetest will) but that same logic doesn't apply to the shared flags (e.g. the default shared flags will almost always have less flags set than the filetest).
abrown submitted PR Review.
abrown created PR Review Comment:
I was under the impression that once the descriptors move into presets then it will be presets from there on?
abrown submitted PR Review.
abrown created PR Review Comment:
And here's an example of the skipped tests on Windows: https://github.com/bytecodealliance/wasmtime/pull/1916/checks?check_run_id=804670568#step:12:652.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
It seems so. It isn't guaranteed anywhere though.
alexcrichton edited PR #1916 from fix-1891
to main
:
This change fixes the issues described in #1891:
- it makes the test-running infrastructure aware of what filetests will not be able to run on the current host machine by means of new implementations of
TargetIsa::is_compatible_with
andFlags::is_compatible_with
(generated)- it fixes the issue @bnjbvr described where the
TargetIsa
created byParser
is unaware of what host it is running on: fortest run
filetests, this is fixed with a use ofTriple::host()
I now feel like
is_compatible_with
(likematches
) is the wrong wording for that function; it should express that one configuration subsumes another so that it is clear that the opposite may not be true. I'm open to suggestions on that. I also likely need to adjust the ordering of commits at some point.
bnjbvr created PR Review Comment:
Generating Rust code from the meta crate has a cost, in particular it's harder to read, correct errors and maintain over time. For a simple function that doesn't seem to use much of the meta machinery anyways, would it make sense to just implement as a plain function in the codegen (non-meta) crate instead? (With the extra argument that really only x86 needs it, so only this ISA can implement right now)
bnjbvr submitted PR Review.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
This check is a bit tricky and might be incorrect: if the setting was explicitly set to the default value, then it's different from being unset and inheriting the default value. This suggests that we'd need a different data structure with
Option
s instead of direct values, and using this to store what's requested by the CLI arguments / test annotations, and comparing this against the real machine capabilities.
bnjbvr created PR Review Comment:
Same comment as below; could this be implemented in Rust with the same effect but making it easier to maintain?
bnjbvr created PR Review Comment:
I'm not sure if the question makes sense, but is there a chance that since the x64 new backend uses a different TargetIsa, this downcast may fail when comparing on a native x86 host?
abrown closed without merge PR #1916.
Last updated: Dec 23 2024 at 12:05 UTC