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

cranelift: add icmp-of-icmp rules for comparisons with 1 #8510

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

tertsdiepraam
Copy link
Contributor

Suggested by @cfallin in this zulip thread: https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/disassemly.20seems.20.28relatively.29.20unoptimized

The icmp-of-icmp rules only included comparisons with 0. So this

(x == y) != 0

gets simplified to

x == y

But the equivalent

(x == y) == 1

did not get simplified, even though the rules to apply are just the inverse of the rules for 0.

I've added the rules for icmp(..) == 1 and icmp(..) != 1 and tried to add some more comments to the existing rules.

I need some help to figure out how I could write a test for this, because I haven't found the tests in the codebase :)

@tertsdiepraam tertsdiepraam requested a review from a team as a code owner April 30, 2024 18:13
@tertsdiepraam tertsdiepraam requested review from cfallin and removed request for a team April 30, 2024 18:13
@tertsdiepraam tertsdiepraam force-pushed the cranelift-icmp-icmp-1 branch 2 times, most recently from 744855e to 8d6dd2c Compare April 30, 2024 18:14
(rule (simplify (eq ty
(uextend_maybe _ (icmp ty cc x y))
(iconst_u _ 0)))
(subsume (icmp ty (intcc_complement cc) x y)))

;; eq(icmp(ty, cc, x, y), 1) == icmp(ty, cc_complement, x, y)
Copy link
Contributor

Choose a reason for hiding this comment

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

are these comments the wrong way around (ie should this be

eq(icmp(ty, cc, x, y), 1) == icmp(ty, cc, x, y)

and similar for the documentation of the other command
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks!

@cfallin
Copy link
Member

cfallin commented Apr 30, 2024

@tertsdiepraam thanks for this! The tests are in cranelift/filetests/filetests/; you can find midend-optimizer-specific tests that assert on expected simplified IR in egraph/, and tests that check execution results in runtests/. Probably both would be good for this case?

@tertsdiepraam
Copy link
Contributor Author

I've added tests both in egraph/ and runtests/ and fixed up the comment. I hope it's good now.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

LGTM -- thanks very much for this contribution!

@cfallin cfallin enabled auto-merge April 30, 2024 19:11
@cfallin cfallin added this pull request to the merge queue Apr 30, 2024
Merged via the queue into bytecodealliance:main with commit 662c35c Apr 30, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants