Skip to content

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

Merged
cfallin merged 1 commit intobytecodealliance:mainfrom
cfallin:issue-5437
Dec 14, 2022
Merged

egraph opt rules: do (icmp cc x x) == {0,1} only for integer types.#5438
cfallin merged 1 commit intobytecodealliance:mainfrom
cfallin:issue-5437

Conversation

@cfallin
Copy link
Copy Markdown
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
Copy Markdown
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