Skip to content
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

egraph opt rules: do (icmp cc x x) == {0,1} only for integer types. #5438

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Dec 14, 2022

We could do these for vectors too, in theory, but for now let's fix the bug by applying the equivalence only for integer types.

Fixes #5437.

@cfallin cfallin requested a review from jameysharp December 14, 2022 19:01
@cfallin cfallin changed the title egraph opt rules: do (icmp x x) == 0 only for integer types. egraph opt rules: do (icmp cc x x) == {0,1} only for integer types. Dec 14, 2022
We could do these for vectors too, in theory, but for now let's fix the
bug by applying the equivalence only for integer types.

Fixes bytecodealliance#5437.
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cfallin cfallin enabled auto-merge (squash) December 14, 2022 19:31
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Dec 14, 2022
@cfallin cfallin merged commit 8383e4b into bytecodealliance:main Dec 14, 2022
@cfallin cfallin deleted the issue-5437 branch December 14, 2022 19:55
cfallin added a commit that referenced this pull request Jan 19, 2023
This PR follows up on #5382 and #5391, which rebuilt the egraph-based optimization framework to be more performant, by enabling it by default.

Based on performance results in #5382 (my measurements on SpiderMonkey and bjorn3's independent confirmation with cg_clif), it seems that this is reasonable to enable. Now that we have been fuzzing compiler configurations with egraph opts (#5388) for 6 weeks, having fixed a few fuzzbugs that came up (#5409, #5420, #5438) and subsequently received no further reports from OSS-Fuzz, I believe it is stable enough to rely on.

This PR enables `use_egraphs`, and also normalizes its meaning: previously it forced optimization (it basically meant "turn on the egraph optimization machinery"), now it runs egraph opts if the opt level indicates (it means "use egraphs to optimize if we are going to optimize"). The conditionals in the top-level pass driver are a little subtle, but will get simpler once we can remove the non-egraph path (which we plan to do eventually!).

Fixes #5181.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unimplemented in ISLE error with egraphs and simd
2 participants