-
Notifications
You must be signed in to change notification settings - Fork 72
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
dialects: (mod_arith) create mod_arith dialect and add initial operation #3218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments but looks like a good start
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3218 +/- ##
==========================================
+ Coverage 89.92% 89.94% +0.02%
==========================================
Files 442 444 +2
Lines 55489 55586 +97
Branches 8656 8675 +19
==========================================
+ Hits 49899 49998 +99
Misses 4161 4161
+ Partials 1429 1427 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome @emmau678! How exciting to see the project grow, this is a very strong start already!
Can you please delete the "Thank you for submitting a PR to xDSL!" bit from the PR description? The PR descriptions get added as commit messages when merged, so it's good to craft these to be helpful when looking at this commit in git history in the future. |
Actually, I believe that everything below the line won't be included. But it probably does not hurt to clean this up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments, probably won't block merging
Co-authored-by: Tobias Grosser <tobias@grosser.es>
Remove the line with `builtin.module` as it is already implied. Move the check line to before instead of after to be consistent with LLVM approach Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
Something's off, did a merge go wrong? |
@superlopuh @alexarice yes, I ran git pull when I should not have, I'm working on it :) |
I think I have fixed it now |
The merge looks fixed. Just the one comment still up then should be good to merge from my end |
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
Create a mod_arith dialect with a basic initial operation, based on the Mod Arith dialect in HEIR.
(link to the original dialect: https://github.com/google/heir/tree/main/lib/Dialect/ModArith)