-
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
transformations: Convert memref
to ptr
dialect
#3383
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3383 +/- ##
=======================================
Coverage 90.13% 90.13%
=======================================
Files 451 452 +1
Lines 56868 56951 +83
Branches 5459 5467 +8
=======================================
+ Hits 51256 51331 +75
- Misses 4167 4173 +6
- Partials 1445 1447 +2 ☔ View full report in Codecov by Sentry. |
) | ||
] | ||
|
||
if len(list(indices)) == 0: |
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.
if len(list(indices)) == 0: | |
if not indices: |
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 the indices passed below are actually a Collection, so this should be possible, and would avoid an extra list construction
def apply(self, ctx: MLContext, op: builtin.ModuleOp) -> None: | ||
the_one_pass = PatternRewriteWalker( | ||
GreedyRewritePatternApplier([ConvertStoreOp(), ConvertLoadOp()]), | ||
apply_recursively=False, |
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.
Is this necessary?
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.
Similarly for the other options below
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.
Could you please add some doc strings to the helpers? LGTM otherwise
from xdsl.utils.exceptions import DiagnosticException | ||
|
||
|
||
# I think we also need to pass the adress width. |
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.
Isn't it computed below?
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, forgot to delete this comment from a previous version
case builtin.StridedLayoutAttr(): | ||
strides = memref_type.layout.get_strides() | ||
case _: | ||
raise DiagnosticException(f"Unsupported layout type {memref_type.layout}") |
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.
Oh I forgot about this, could you please add a small PR with a test that catches this diagnostic exception?
Introducing lowering of some memref operations to a ptr dialect. This will be used to translate riscv compilation to ptr dialect in the future (this is the second pr in the series).
Introducing lowering of some memref operations to a ptr dialect. This will be used to translate riscv compilation to ptr dialect in the future (this is the second pr in the series).