Skip to content
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

dialects: (csl) Data structure descriptors #2648

Merged
merged 7 commits into from
May 31, 2024
Merged

dialects: (csl) Data structure descriptors #2648

merged 7 commits into from
May 31, 2024

Conversation

n-io
Copy link
Collaborator

@n-io n-io commented May 28, 2024

A basic implementation of dsd types and the @get_dsd. Custom array index expressions are not yet supported, and the current version should be understood (and printed) as accessing A[i,j,k] according to the number of dimensions and generate induction vars accordingly.

@n-io n-io changed the title csl: Data structure descriptors csl: data structure descriptors May 28, 2024
@n-io n-io requested review from dk949 and AntonLydike May 28, 2024 13:21
Copy link

codecov bot commented May 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.62%. Comparing base (6a1643d) to head (7c30803).
Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2648      +/-   ##
==========================================
+ Coverage   89.60%   89.62%   +0.01%     
==========================================
  Files         360      364       +4     
  Lines       46179    46658     +479     
  Branches     6978     7057      +79     
==========================================
+ Hits        41378    41816     +438     
- Misses       3726     3748      +22     
- Partials     1075     1094      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xdsl/dialects/csl.py Outdated Show resolved Hide resolved
xdsl/dialects/csl.py Outdated Show resolved Hide resolved
@dk949
Copy link
Collaborator

dk949 commented May 28, 2024

Would fabin_dsd and fabout_dsd be part of the same type and op, or should they have their own?

@n-io
Copy link
Collaborator Author

n-io commented May 28, 2024

Would fabin_dsd and fabout_dsd be part of the same type and op, or should they have their own?

Since they need different fields specified, making it a separate op is actually a viable option.

@dk949
Copy link
Collaborator

dk949 commented May 28, 2024

Would fabin_dsd and fabout_dsd be part of the same type and op, or should they have their own?

Since they need different fields specified, making it a separate op is actually a viable option.

In that case it would make sense to rename this op (and class) something like csl.get_mem_dsd.

I think it makes sense for both ops to still return a csl.dsd, since all the maths builtins accept either a mem dsd or a fab dsd.

EDIT: by both ops I mean this and the hypothetical future csl.get_fab_dsd.

@superlopuh superlopuh changed the title csl: data structure descriptors dialects: (csl) data structure descriptors May 29, 2024
@superlopuh superlopuh changed the title dialects: (csl) data structure descriptors dialects: (csl) Data structure descriptors May 29, 2024
Copy link
Collaborator

@AntonLydike AntonLydike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is all that is needed to get DSDs supported in xDSL then that's amazing! I'm afraid that I don't know much about the internals of DSDs though, so for that @dk949 is probably the better conversation partner!

From my side this looks good!

@@ -70,6 +70,10 @@ csl.func @initialize() {
%many_arr_ptr = "csl.addressof"(%arr) : (memref<10xf32>) -> !csl.ptr<f32, #csl<ptr_kind many>, #csl<ptr_const const>>
%single_arr_ptr = "csl.addressof"(%arr) : (memref<10xf32>) -> !csl.ptr<memref<10xf32>, #csl<ptr_kind single>, #csl<ptr_const const>>

%dsd_1d = "csl.get_dsd"(%arr, %scalar) : (memref<10xf32>, i32) -> !csl<dsd mem1d_dsd>
%dsd_2d = "csl.get_dsd"(%arr, %scalar, %scalar) : (memref<10xf32>, i32, i32) -> !csl<dsd mem4d_dsd>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions here:

  1. Can we not just read the sizes from the memref type?
  2. Can we have two sizes passed to the get_dsd if the underlying array is 1D?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What if you don't want to iterate over the entire buffer?
  2. It will fail to verify.

@n-io
Copy link
Collaborator Author

n-io commented May 30, 2024

Would fabin_dsd and fabout_dsd be part of the same type and op, or should they have their own?

Since they need different fields specified, making it a separate op is actually a viable option.

In that case it would make sense to rename this op (and class) something like csl.get_mem_dsd.

I think it makes sense for both ops to still return a csl.dsd, since all the maths builtins accept either a mem dsd or a fab dsd.

EDIT: by both ops I mean this and the hypothetical future csl.get_fab_dsd.

I've added fabin_dsd and fabout_dsd into the same get_dsd op. It ended up with a lot of the fields being optional and needs verification that all (+only) the right fields are populated.

@n-io
Copy link
Collaborator Author

n-io commented May 31, 2024

GetDsdOp is now an abstract base class and there are subclasses for csl.get_mem_dsd and csl.get_fab_dsd. Overall, this looks a lot cleaner.

@n-io n-io merged commit 838c8ea into main May 31, 2024
10 checks passed
@n-io n-io deleted the nicolai/csl-dsds branch May 31, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants