[SM6.10][HLK] LinAlg element access exec tests#8312
[SM6.10][HLK] LinAlg element access exec tests#8312V-FEXrt wants to merge 4 commits intomicrosoft:mainfrom
Conversation
You can test this locally with the following command:git-clang-format --diff c6893856a7391c9e7a242ea6a028bbee6ed16205 54dad8f99be32ba259a4ef6b4373bd77b33496a1 -- tools/clang/unittests/HLSLExec/LinAlgTests.cppView the diff from clang-format here.diff --git a/tools/clang/unittests/HLSLExec/LinAlgTests.cpp b/tools/clang/unittests/HLSLExec/LinAlgTests.cpp
index 06ca264c..44f6e0d1 100644
--- a/tools/clang/unittests/HLSLExec/LinAlgTests.cpp
+++ b/tools/clang/unittests/HLSLExec/LinAlgTests.cpp
@@ -505,7 +505,8 @@ static const char ElementAccessShader[] = R"(
static void runElementAccess(ID3D12Device *Device,
dxc::SpecificDllLoader &DxcSupport,
- const MatrixParams &Params, int MajorDim, bool Verbose) {
+ const MatrixParams &Params, int MajorDim,
+ bool Verbose) {
const size_t NumElements = Params.totalElements();
const size_t NumThreads = Params.NumThreads;
const size_t InputBufSize = Params.totalBytes();
|
| // 0,1 = 4 | ||
| // 1,0 = 16 | ||
| // 3,3 = 60 | ||
| // TODO: this assumes M=4,N=4 |
There was a problem hiding this comment.
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
- All individual elements read/written to the buffer are 4 bytes wide (float/int)
- All int results being read back for verification are <256 bytes (it only checks a single byte index of all 4)
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.
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.
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.
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.
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.
hmm nope, I just checked the spec and the minimum is 4x4!
There was a problem hiding this comment.
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.
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.
16x16x16 with F16 types 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.
|
Attempting to fixing GH issues! will reopen in a moment |
|
#8317 updated PR |
Adds HLK smoke test for LinAlg matrix element access