-
Notifications
You must be signed in to change notification settings - Fork 80
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
interactive: add rewrite functionality to gui tool 3/3 #2155
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2155 +/- ##
========================================
Coverage 89.37% 89.37%
========================================
Files 310 310
Lines 37982 38101 +119
Branches 5607 5626 +19
========================================
+ Hits 33945 34053 +108
- Misses 3256 3261 +5
- Partials 781 787 +6 ☔ View full report in Codecov by Sentry. |
rewrite_spec = list( | ||
parse_pipeline(f"{rewrite_pass.name}{{{rewrite_spec_arg_str}}}") | ||
)[0] | ||
op = list(self.current_module.walk())[op_idx] |
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.
it looks like we only use the op to get its description, maybe we'd be better off getting that as part of the available rewrites? It also feels wasteful to get the nth op for every op if we already had this information in the past
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.
I add the op to what "get_all_possible_rewrites" returns. what do u think? does this make sense?
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.
I considered removing the operation index, but I think we should leave it for now as it could be useful for future plans we have for functionalities (line numbers etc.)
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.
@superlopuh I realize now that maybe it is better to leave it the way that it is.
returning the str(Operation)
from get_all_possible_rewrites():
When the rewrite is displayed in the gui, we now get
%0 = arith.addi %1, %2
instead of
Addi(%res = arith.addi %two, %n : i32)
returning the Operation
from get_all_possible_rewrites() - (This wont work either way due to issues with testing and equality checking of an Operation):
When the rewrite is displayed in the gui, we now get
Addi(%0 = arith.addi %1, %2)
instead of
Addi(%res = arith.addi %two, %n : i32)
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.
#2162
@superlopuh told me to file an issue. we will address this later. will merge as is.
attempt to split t[his PR](https://github.com/xdslproject/xdsl/pull/2131/files#diff-6c0fd122274ca87c3f88d46a4abe86a2d09289633359f7c9830925aeb2137178L56) into smaller ones. PR 3 - adjust compute_available_pass_list to compute all_possible_rewrites for given module and wrap them in an "individual rewrite pass" i.e. rewrites now passes - adjust get_pass_arguments function accordingly - add a test that makes sure rewrite functionality in gui tool works
attempt to split this PR into smaller ones.
PR 3