Skip to content

Rewrite rule to remove redundant constants #251

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

Closed
wants to merge 2 commits into from
Closed

Rewrite rule to remove redundant constants #251

wants to merge 2 commits into from

Conversation

kaihsin
Copy link
Contributor

@kaihsin kaihsin commented Feb 16, 2025

This PR add rewrite pass to eliminate redundant Constant.

Related #239

@kaihsin kaihsin changed the title Rewrite pass to remove redundant constants Rewrite rule to remove redundant constants Feb 16, 2025
Copy link

codecov bot commented Feb 16, 2025

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/kirin/rewrite/gve.py 94.73% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@kaihsin kaihsin requested a review from Roger-luo February 16, 2025 22:12
Copy link
Contributor

github-actions bot commented Feb 16, 2025

PR Preview Action v1.6.0
Preview removed because the pull request was closed.
2025-02-17 01:14 UTC

Copy link
Contributor

github-actions bot commented Feb 16, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
8724 7683 88% 0% 🟢

New Files

File Coverage Status
src/kirin/rewrite/gve.py 95% 🟢
TOTAL 95% 🟢

Modified Files

No covered modified files...

updated for commit: b4d3ad4 by action🐍

@Roger-luo
Copy link
Member

can you explain why your implementation address GVN and not the same as CSE? It seems to me the test case you propose here can also be eliminated by CSE?

@kaihsin
Copy link
Contributor Author

kaihsin commented Feb 17, 2025

oh right, this is covered by CSE. At some point the constant did not get CSE. Then let's close this one.

Also, I think the second test example catch a bug in CSE. (but let's deal it in another PR)
since 1.0 (float) and 1 (int) gives the same hash value, so they got CSE'd even tho they are different type.

@kaihsin kaihsin closed this Feb 17, 2025
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.

2 participants