-
Notifications
You must be signed in to change notification settings - Fork 79
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
backend: (csl) Add printing for CSL dialect ops #2593
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## anton/csl-dialect-1 #2593 +/- ##
=======================================================
- Coverage 89.65% 89.64% -0.01%
=======================================================
Files 358 358
Lines 45325 45353 +28
Branches 6815 6826 +11
=======================================================
+ Hits 40634 40655 +21
- Misses 3663 3666 +3
- Partials 1028 1032 +4 ☔ View full report in Codecov by Sentry. |
|
||
def _get_variable_name_for(self, val: SSAValue) -> str: | ||
def _get_variable_name_for(self, val: SSAValue, hint: str | None = None) -> str: |
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'm not sure how much we want to rely on name hints for naming variables in the output, especially now that we mess with them at parsing time, like stripping _0
, _1
, ... suffixes. The two alternatives that come to mind is either a custom type for variables or an attribute on ops that own the variables. The second seems vaguely preferable. I don't want to derail your csl mergeathon too much, but if you agree that not relying on name hints is a good change in the future, it would be great to file a PR and mention it in a comment.
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.
Yeah, not sure how we want to continue to go on with name hints from here. I'd see how it evolves over time, maybe we don't actually need readable CSL (the official examples don't seem to bother too much with that either).
This PR adds printing for the CSL dialect operations (stacked on top of #2591)