Add an overlap checker to ISLE #4906

elliottt · 2022-09-13T16:16:40Z

Introduce overlap checking for ISLE as the start of addressing #4717.

The overlap checker detects cases where ISLE rules could fire for the same input. Consider the following example from the x64 lowerings (line numbers added for clarity):

13: (rule (sse_xor_op $F32X4) (SseOpcode.Xorps))
14: (rule (sse_xor_op $F64X2) (SseOpcode.Xorpd))
15: (rule (sse_xor_op (multi_lane _bits _lanes)) (SseOpcode.Pxor))

Rule 15 overlaps separately with rules 13 and 14, but rules 13 and 14 don't overlap with each other. With overlap checking enabled this will be reported as an overlap of all three rules, where the rule on line 15 will be identified as the source of the overlap (error messages could definitely be improved here). In this case the overlap can be resolved by setting a lower priority for rule 3.

Error:
  × overlap error: rules are overlapping
  │   ../x64.isle:15:0
  │   ../x64.isle:13:0
  │   ../x64.isle:14:0

For another example, consider the following:

1: (rule (example $F32 25) ...)
2: (rule (example $F32 (fallible_extractor y)) ...)
3: (rule (example $F64 _) ...)

In this case, only the rules on lines 1 and 2 overlap, as we don't know that the fallible extractor won't fire successfully on the input 25. As the rule on line 3 has a different leading pattern ($F64) it won't be part of the overlap group generated.

However, if the example looks instead like the following:

1: (rule (example $F32 (fallible_extractor 25)) ...)
2: (rule (example $F32 (fallible_extractor 31)) ...)
3: (rule (example $F64 _) ...)

We consider there to be no overlap in the rules as fallible extractors are expected to be pure.

elliottt · 2022-09-13T17:15:25Z

cranelift/isle/isle/src/error.rs

+            Error::OverlapError { msg, rules, .. } => {
+                writeln!(f, "overlap error: {}\n{}", msg, OverlappingRules(&rules))
+            }


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.

jameysharp

I haven't gotten to reviewing the actual overlap checking, but here are some initial thoughts.

cranelift/isle/isle/src/compile.rs

cranelift/isle/isle/src/overlap.rs

jameysharp · 2022-09-13T17:53:27Z

cranelift/isle/isle/src/overlap.rs

+        match self.get_term(id).kind {
+            TermKind::Decl {
+                constructor_kind: Some(ConstructorKind::InternalConstructor),
+                ..
+            } => true,
+            _ => false,
+        }


I'm not sure whether it's an improvement here, but there's a handy macro in the standard library for this pattern:

Suggested change

match self.get_term(id).kind {

TermKind::Decl {

constructor_kind: Some(ConstructorKind::InternalConstructor),

..

} => true,

_ => false,

}

matches!(

self.get_term(id).kind,

TermKind::Decl {

constructor_kind: Some(ConstructorKind::InternalConstructor),

..

},

)

I could go either way on this, do you feel strongly about switching to the macro?

I do not feel strongly about that, nope!

cranelift/isle/isle/src/overlap.rs

github-actions · 2022-09-13T19:08:16Z

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "isle"

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

cfallin: isle
fitzgen: isle

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

Learn more.

elliottt · 2022-09-15T19:58:14Z

cranelift/isle/isle/src/overlap.rs

+
+/// The patterns of a rule turned into a work-queue for overlap checking.
+#[derive(Debug, Clone)]
+struct Row {


I think this is a bad name for this type now, how about Patterns?

jameysharp · 2022-09-16T17:22:06Z

cranelift/isle/isle/src/overlap.rs

+        match self.get_term(id).kind {
+            TermKind::Decl {
+                constructor_kind: Some(ConstructorKind::InternalConstructor),
+                ..
+            } => true,
+            _ => false,
+        }


I do not feel strongly about that, nope!

jameysharp · 2022-09-16T17:23:12Z

cranelift/isle/isle/src/overlap.rs

+    }
+
+    /// The ids of all [`Rule`]s defined for this term.
+    fn rules_for_term(&self, id: TermId) -> Vec<RuleId> {


This is immediately converted back to an iterator by its only caller, so I'd return impl Iterator<Item = RuleId> instead of calling .collect().

jameysharp · 2022-09-16T17:29:58Z

cranelift/isle/isle/src/overlap.rs

+    /// Enum variant constructors.
+    Variant {
+        id: TermId,
+        single_case: bool,


As far as I can tell, this field is never read and can be removed, along with the is_single_constructor_enum method. I'm puzzled why you didn't get a build warning about this...

Once you do that I believe Variant and Extractor are treated the same way everywhere so you can merge them. I'm assuming that the same TermId is never used both for an enum variant and for an extractor.

As far as I can tell, this field is never read and can be removed, along with the is_single_constructor_enum method. I'm puzzled why you didn't get a build warning about this...

This was originally when I was going to try to reason about the interaction of single-constructor enums to know that no other case was ever possible. I'm not sure there's anything we can do with that information now, so it should be removed.

Once you do that I believe Variant and Extractor are treated the same way everywhere so you can merge them. I'm assuming that the same TermId is never used both for an enum variant and for an extractor.

We'd still need to be able to differentiate between a Variant and an Extractor for the case where the match fails: in that case the variant means that the two don't overlap, while the extractor could still potentially fire.

Yeah, I figured that out after writing this comment. 😅

This also puts all rule-pairs in a single list that rayon can efficiently partition across multiple CPUs. I previously suggested two layers of par_iters and relying on work-stealing to balance things out, but this is simpler to read and also ought to work more smoothly.

cfallin

This looks great overall -- I just have one question below regarding and-patterns:

cranelift/isle/isle/src/overlap.rs

bjorn3 · 2022-10-23T11:53:11Z

cranelift/isle/isle/Cargo.toml

@@ -11,12 +11,14 @@ version = "0.89.0"
 [dependencies]
 log = { version = "0.4", optional = true }
 miette = { version = "5.1.0", optional = true }
+rayon = "^1.5"


This adds a lot of dependencies to Cranelift. Is the overlap checking really so expensive that paralleling it makes a lot of difference?

I just checked it. Takes less than half a second total even when not parallelizing. Will open a PR to remove rayon.

We depend on rayon elsewhere in cranelift, which is why we added it to the overlap checker. Were you removing it completely from cranelift?

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.

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.

FWIW cranelift/Cargo.toml defines the crate for cranelift-tools, the CLI utility; we do want to keep dependencies of cranelift-codegen as minimal as possible, and since cranelift-isle is a build-dep of that I'd consider the same policy to apply. Sorry for not catching this earlier!

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.

elliottt requested a review from cfallin September 13, 2022 17:14

elliottt marked this pull request as ready for review September 13, 2022 17:14

elliottt commented Sep 13, 2022

View reviewed changes

elliottt changed the title ~~wip: Add an overlap checker to ISLE~~ Add an overlap checker to ISLE Sep 13, 2022

jameysharp reviewed Sep 13, 2022

View reviewed changes

github-actions bot added cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language labels Sep 13, 2022

elliottt marked this pull request as draft September 14, 2022 17:22

elliottt and others added 12 commits September 15, 2022 10:46

Add an overlap checker to ISLE

d9235c6

Fix comments

992ce46

Minimize diff

2871a9c

Add more comments

9906df0

Expand comments about handling non-linear patterns

15283d3

Refactor the overlap checker to process pairs of rules

84de3dd

Checkpoint

1e7ef90

Improve errors consumption

e9f26d2

Collect errors from the highest degree node at each iteration

443066d

Don't register overlap for rules of different priorities

49424cc

More fold/reduce instead of sequential reduction

8e48ca2

Always run the overlap checker, but add a feature for raising errors

2c0ea5a

elliottt force-pushed the trevor/isle-overlap-checker branch from 2aa1fca to 2c0ea5a Compare September 15, 2022 18:41

elliottt added 5 commits September 15, 2022 12:11

Fix spelling error

ff6c081

Format

f184e07

Fix some comments and function names

75ed9b9

Remove the remove_node function on Errors

7173097

The collected errors no longer need to be mutable

b42eab9

elliottt commented Sep 15, 2022

View reviewed changes

elliottt marked this pull request as ready for review September 15, 2022 21:18

jameysharp reviewed Sep 16, 2022

View reviewed changes

elliottt and others added 10 commits September 16, 2022 11:47

Reverse the row patterns after constructing them

353880b

Remove the single_case field on Pattern::Variant

ac09ba2

Simplify by removing some abstractions

ae7cb60

Remove nodes as soon as they have no edges left

6534a84

Iterate over values instead of looking them up by key

c97fe05

Update comments

e0339a9

Remove Env wrapper in favor of TypeEnv/TermEnv

c5490c0

Remove Node abstraction

569b3f5

Use boxed slices instead of Vec for immutable arrays

01b078b

cfallin reviewed Sep 21, 2022

View reviewed changes

cranelift/isle/isle/src/overlap.rs Show resolved Hide resolved

cfallin approved these changes Sep 21, 2022

View reviewed changes

elliottt merged commit b167172 into bytecodealliance:main Sep 21, 2022

bjorn3 reviewed Oct 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an overlap checker to ISLE #4906

Add an overlap checker to ISLE #4906

elliottt commented Sep 13, 2022 •

edited

Loading

elliottt Sep 13, 2022

jameysharp left a comment

jameysharp Sep 13, 2022

elliottt Sep 15, 2022

jameysharp Sep 16, 2022

github-actions bot commented Sep 13, 2022

elliottt Sep 15, 2022

jameysharp Sep 16, 2022

jameysharp Sep 16, 2022

jameysharp Sep 16, 2022

elliottt Sep 16, 2022

jameysharp Sep 16, 2022

cfallin left a comment

bjorn3 Oct 23, 2022

bjorn3 Oct 23, 2022

elliottt Oct 23, 2022

bjorn3 Oct 23, 2022

elliottt Oct 23, 2022

cfallin Oct 23, 2022

jameysharp Oct 24, 2022

Add an overlap checker to ISLE #4906

Add an overlap checker to ISLE #4906

Conversation

elliottt commented Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

jameysharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 13, 2022

Subscribe to Label Action

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfallin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elliottt commented Sep 13, 2022 •

edited

Loading