-
Notifications
You must be signed in to change notification settings - Fork 83
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
transformations: (convert-ptr-to-llvm) add lowering for ptr to llvm #3825
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 27bf821.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3825 +/- ##
==========================================
- Coverage 91.25% 91.16% -0.10%
==========================================
Files 461 462 +1
Lines 57630 57748 +118
Branches 5567 5587 +20
==========================================
+ Hits 52591 52644 +53
- Misses 3612 3675 +63
- Partials 1427 1429 +2 ☔ View full report in Codecov by Sentry. |
good stuff overall; I wouldn't worry about the unsquashed commits. |
class ConvertPtrAddOp(RewritePattern): | ||
@op_type_rewrite_pattern | ||
def match_and_rewrite(self, op: ptr.PtrAddOp, rewriter: PatternRewriter, /): | ||
# TODO: could be one cast? |
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.
please remove all todos in this file, either by addressing them, or by filing issues, let's discuss this
class ConvertTypeOffsetOp(RewritePattern): | ||
@op_type_rewrite_pattern | ||
def match_and_rewrite(self, op: ptr.TypeOffsetOp, rewriter: PatternRewriter, /): | ||
# TODO: replace with memoized dict lookup |
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.
we have a helper for this, it should just be op.elem_type.size
if it's a FixedBitwidthType
, we might want to raise an error otherwise.
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.
Btw I merged the type offset conversion pass so maybe you can just use it in this case
ConvertLoadOp(), | ||
ConvertTypeOffsetOp(), | ||
ConvertPtrAddOp(), | ||
# TODO: could maybe use type rewriting? |
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.
let's try that
Let's iterate on this? It would be good to merge everything you used, and throw away the rest |
WIP: Adds a pass,
convert-ptr-to-llvm
, for lowering theptr_xdsl
dialect tollvm
.TODO
ptr
?There's also a revert to 27bf821 as it breaks
xdsl-opt
.