-
Notifications
You must be signed in to change notification settings - Fork 25
[Matrix] Add single subscript test #503
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
Conversation
https://godbolt.org/z/xaEE44cbE Test 3 cases (loads and stores): 1. single subscript as an l-value swizzle 2. single subscript where the store/load takes a vector r-value swizzle 3. single subscript where we store/load the vector as is.
| In[15]); | ||
|
|
||
| for (int i = 0; i < 4; i++) { | ||
| int4 B; |
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.
very minor nit, but a for loop for 4 assignments with two of them be 'special' made me look at this for a moment longer.
4 explicit assignment statements might be cleaner.
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.
its going to be much more than 4 explicit asignments because its 16 asignments to fill up Out[i*4 + j].
alsepkow
left a comment
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. Minor nit on the first for loop in both tests. Take it or leave it.
Icohedron
left a comment
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! Just a few formatting/spacing nits
tex3d
left a comment
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.
Minor comments/suggestions, nothing necessarily requiring changes regarding:
- dynamic/static subscript indexing and potential optimizations
- symmetrical swizzles (equivalent when used in source or dest)
| for (int i = 0; i < 4; i++) { | ||
| int4 B; | ||
| if (i % 2 == 0) | ||
| B = A[i]; |
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.
Just a comment, not a request for change:
These subscript accesses look dynamic, but these loops are likely to be unrolled, either before IR (in DXC, Clang, etc.) or after (in a driver compiler). That unrolling would make all accesses static, even eliminating the intermediate local variables to copy values directly from static indices of In to static indices of Out. That's a fine path to test, but this might not test other important dynamic indexing code paths, if desired.
I guess this gets at the fuzzy purpose issue with the offload-test-suite. This seems fine to test this scenario end-to-end, whatever path it may take through optimizations, and maybe that's all that's desired for this test.
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 was trying to use uint GI : SV_GroupIndex for the input to main because I thought someone might mention that. But I could not get the persistance I needed to properly fill the out buffer.
In anycase the matrix execution tests are like you said more about confirming a matrix frontend features behave the same as they would on DXC and thus a little bit different than some of the other behaviors being tested in the offload test suite like intrinsic tests. In my case its more about layout and access of data which should be preserved regardless of optimization passes.
fixes #502
https://godbolt.org/z/xaEE44cbE
Test 3 cases (loads and stores):