-
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
transformations: Translate memref to dsd #3092
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3092 +/- ##
==========================================
- Coverage 89.91% 89.89% -0.02%
==========================================
Files 419 421 +2
Lines 53070 53252 +182
Branches 8226 8256 +30
==========================================
+ Hits 47718 47871 +153
- Misses 4023 4042 +19
- Partials 1329 1339 +10 ☔ View full report in Codecov by Sentry. |
1ee9981
to
4fe880f
Compare
xdsl/transforms/memref_to_dsd.py
Outdated
|
||
# update offsets only if they differ from op.source.type | ||
if op.static_offsets.data.data[0].data == memref.Subview.DYNAMIC_INDEX: | ||
pass # todo |
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.
🤔
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.
What happens if we hit this? What is to be done?
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.
basically the same as for static, working on this now. If the value is (directly) from an arith const, we can check if we need to generate the offset/stride/size dsd instruction. Mostly, I suppose, we have to act blindly as one does with dynamic values. However, there's nothing in our pipeline that actually generates dynamic values here, so it's purely optical and not actually 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.
I came to think, if we have a dynamic value, let's assume it is not as de-facto static as coming from an arith.constant (in this case, we should find or write a pass to promote arith.constant to static values). It's probably simpler and cleaner to not analyse dynamic values here and simply generate dsd modify ops for each dynamic value, without analysis.
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 think it all makes sense
Translate memref to csl dsd ops.
Todo: Finalise how global buffers are stored.