-
Notifications
You must be signed in to change notification settings - Fork 851
[SM6.10][HLK] LinAlg element access exec tests #8312
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
Closed
Closed
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
A lot of this test (pre and post copilot rewrite) makes a couple assumptions that won't really expand to full exec tests.
Specifically, this test (implictly) assumes 4x4 matrix in a couple places and more generally the test framework assumes the following
The last point is the easiest to fix, but in general I think my stance is that we punt on fixing these until all the smoke tests are in that way we can iteratively solve the chicken-egg problem. If we make the tests any more complicated I feel we'll risk major bugs in the 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 think at minimum, we would want the smoke test to fit within required Tier 1 functionality, which might require different M & N dimensions. Implicitly requiring M=4,N=4 sounds like a bad starting assumption.
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 I'm not very excited about the 4x4 assumption at all and that should be fixed but I don't think we want quite as far as having tests for different M & N dims just yet.
My goal right now is to just exercise every operation with the most basic possible test. Since we can't actually run anything, every level of complexity on top of that is something that'll increase chances of a test bug in the code for our partners to discover during their bring up
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’s not required to try variable dimensions, but if you choose 16x16, and put those in a #define or static constant, that would be fine.
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 improved the structure of the test, and I'm a lot happier with that but I'm not realizing you may have been saying the minimum matrix size is 16x16 and we shouldn't even have 4x4 in the 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.
hmm nope, I just checked the spec and the minimum is 4x4!
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'm saying that 16x16 should be supported as part of required functionality, while 4x4 would be optional depending on what the device reports as supported (and we don't yet have that infrastructure finalized or implemented), so it's likely it won't be usable on real hardware, while 16x16 should be.
In other words, a smoke test shouldn't test a corner case that most real devices can't run.
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.
On that note also: I think F16 for all types would be within the required set, while F32 and I32 are not.
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.
16x16x16withF16types will always be a supported combination within Tier 1 requirements, so I'm recommending we use that for the smoke test, rather than optional dimensions/component types.