-
Notifications
You must be signed in to change notification settings - Fork 77
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) Maths and memory operation printing #2676
Conversation
Extracted printing logic for functions and tasks into one function. If a task can be immediately bound (i.e. it has an ID attribute), it will be. Also added printing for csl.call
This part operates on the program module and prints `@export_symbol`.
This also provides the second half of the symbol exporting logic (i.e. `@export_name` in the layout file).
Not all of these ops and types are implemented in the current printer, but this tests for everything the old branch implemented.
Also test for control tasks
Not a list of it's ops
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2676 +/- ##
==========================================
- Coverage 89.66% 89.66% -0.01%
==========================================
Files 364 364
Lines 46731 46823 +92
Branches 7081 7102 +21
==========================================
+ Hits 41900 41982 +82
- Misses 3738 3739 +1
- Partials 1093 1102 +9 ☔ 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.
LGTM overall, just a few minor nits
%constI32 = arith.constant 0 : i32 | ||
%constU16 = arith.constant 0 : ui16 | ||
%constF32 = arith.constant 0 : f32 | ||
%castIndex = "arith.index_cast"(%constF32) : (f32) -> index |
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.
index_cast
can only cast between index and integer data types I'm afraid
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.
Not sure why this isn't checked by xDSL
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 didn't realise that. Fixed
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.
PR open #2682 btw :)
xdsl/backend/csl/print_csl.py
Outdated
assert isinstance( | ||
ty, csl.PtrType | ||
), f"Result of {csl.AddressOfOp.name} has to be a pointer" |
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.
This is checked by the verify
(although I guess this is needed to make pyright happy?). Not sure we need the assert here, I think we could replace it by a cast
, as it is never going to be wrong?
I guess this is a trade-off between more readable printing code vs. better error messages when printing invalid csl code...
I guess imo we don't need this assertion here.
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.
Yes, this assert is just for the sake of the type checker, it does get caught by verify
. A cast
should be safe here.
PtrType
FunctionType
IndexCastOp
SIToFPOp
FPToSIOp
ExtFOp
TruncFOp
TruncIOp
ExtSIOp
ExtUIOp
Muli
Mulf
Addi
Addf
AddressOfOp
Store
Load