-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an overlap checker to ISLE #4906
Changes from all commits
d9235c6
992ce46
2871a9c
9906df0
15283d3
84de3dd
1e7ef90
e9f26d2
443066d
49424cc
8e48ca2
2c0ea5a
ff6c081
f184e07
75ed9b9
7173097
b42eab9
353880b
ac09ba2
ae7cb60
b702e46
6534a84
c97fe05
e0339a9
c5490c0
569b3f5
01b078b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,17 @@ pub enum Error { | |
span: Span, | ||
}, | ||
|
||
/// The rules mentioned overlap in the input they accept. | ||
OverlapError { | ||
/// The error message. | ||
msg: String, | ||
|
||
/// The locations of all the rules that overlap. When there are more than two rules | ||
/// present, the first rule is the one with the most overlaps (likely a fall-through | ||
/// wildcard case). | ||
rules: Vec<(Source, Span)>, | ||
}, | ||
|
||
/// Multiple errors. | ||
Errors(Vec<Error>), | ||
} | ||
|
@@ -108,6 +119,10 @@ impl std::fmt::Display for Error { | |
#[cfg(feature = "miette-errors")] | ||
Error::TypeError { msg, .. } => write!(f, "type error: {}", msg), | ||
|
||
Error::OverlapError { msg, rules, .. } => { | ||
writeln!(f, "overlap error: {}\n{}", msg, OverlappingRules(&rules)) | ||
} | ||
Comment on lines
+122
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not the most helpful error output, but I'm assuming that as we start burning down the list of overlaps we'll figure out what's most useful to identify. |
||
|
||
Error::Errors(_) => write!( | ||
f, | ||
"found {} errors:\n\n{}", | ||
|
@@ -128,6 +143,16 @@ impl std::fmt::Display for DisplayErrors<'_> { | |
} | ||
} | ||
|
||
struct OverlappingRules<'a>(&'a [(Source, Span)]); | ||
impl std::fmt::Display for OverlappingRules<'_> { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
for (src, span) in self.0 { | ||
writeln!(f, " {}", span.from.pretty_print_with_filename(&*src.name))?; | ||
} | ||
Ok(()) | ||
} | ||
} | ||
|
||
/// A source file and its contents. | ||
#[derive(Clone)] | ||
pub struct Source { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a lot of dependencies to Cranelift. Is the overlap checking really so expensive that paralleling it makes a lot of difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked it. Takes less than half a second total even when not parallelizing. Will open a PR to remove rayon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We depend on rayon elsewhere in cranelift, which is why we added it to the overlap checker. Were you removing it completely from cranelift?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasmtime-cranelift depends on rayon, but cranelift itself doesn't apart from cranelift-isle because of this PR. bjorn3/rustc_codegen_cranelift@266e967 reverted cg_clif back to cranelift 0.88.1 from before this PR. This commit removes rayon from cg_clif's Cargo.lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. My assumption had been that since it was in
cranelift/Cargo.toml
that it would be okay to rely on it in isle.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW
cranelift/Cargo.toml
defines the crate forcranelift-tools
, the CLI utility; we do want to keep dependencies ofcranelift-codegen
as minimal as possible, and sincecranelift-isle
is a build-dep of that I'd consider the same policy to apply. Sorry for not catching this earlier!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, when we added the rayon dependency, overlap checking was dramatically slower, so parallelism shaved a lot of time off the wasmtime build. But by the time we actually merged overlap checking to
main
, we'd fixed its performance, so rayon didn't matter any more; we just didn't re-evaluate it at that point.So thank you for #5101: it's a good change.