Add GroupMemoryBarrierWithGroupSync tests
#442
Conversation
| - Name: ExpectedOut | ||
| Format: Int32 | ||
| Stride: 16 | ||
| Data: [ 9, 90, 900, 9000, 9, 90, 1800, 18000, 10, 20, 120, 1120, 1, 1000, 100, 10 ] |
There was a problem hiding this comment.
I think it's clearer if you split the lines so that the components of the vector each get their own line
| Data: [ 9, 90, 900, 9000, 9, 90, 1800, 18000, 10, 20, 120, 1120, 1, 1000, 100, 10 ] | |
| Data: [ | |
| 9, 90, 900, 9000, | |
| 9, 90, 1800, 18000, | |
| 10, 20, 120, 1120, | |
| 1, 1000, 100, 10 | |
| ] |
| DispatchSize: [1, 1, 1] | ||
| Buffers: | ||
| - Name: In | ||
| Format: Int32 |
There was a problem hiding this comment.
The HLSL uses uint for each of the buffers, but here we say Int32 - we should either make the HLSL used signed ints or use UInt32 here.
| Buffers: | ||
| - Name: In | ||
| Format: Int32 | ||
| Stride: 4 |
There was a problem hiding this comment.
"Stride" is in bytes, and since the elements here are uint4 vectors that means that this should be 16. In any case, Stride is really for structs, and you can just use "Channels: 4" here to say we have 4-element vectors.
| Data: [ 1, 10, 100, 1000] | ||
| - Name: Out | ||
| Format: Int32 | ||
| Stride: 16 |
There was a problem hiding this comment.
Same here (though this stride matches the data type), better to just say "Channels: 4" instead of specifying stride.
There was a problem hiding this comment.
We've been discussing offline, but I figured I'd get something into the record to see if @bogner or anyone else has opinions.
It looks like this test passes if you remove all of the barriers from the shader. This isn't surprising, because with only 4 threads per-thread group there's not really anything for the barrier to synchronize.
We should come up with something where it'll only pass if the barriers are actually in place.
it was noted that if the group size is not larger than the wave size, the barrier option is redundant HLSL waves size can be at most 128. Hence, we initialize a group of 512 threads so that it is forced to evaluate over multiple waves. We also remove the divergent test case as this is not applicable due to it being undefined behaviour
Co-authored-by: Justin Bogner <[email protected]>
|
I have updated the test-cases such that if you remove the barriers it fails locally for me using WARP. I had to increase the group size such that is larger than a wave size, and then it will depend on how the waves are scheduled to get the datarace. But it at least consistently fails for me. |
| // Strided Read/Write: | ||
| Indices[ThreadID.x][ThreadID.y] = ThreadID.x + ThreadID.y * 128; |
There was a problem hiding this comment.
Do we really need any more tests than we have after Out[1]? Arguably Out[0] is enough too, but it does seem that the other one is a bit more interesting.
inbelic
added
the
test-all
inbelic
removed
the
test-all
Adds some basic testing of blocking functionality with the
GroupMemoryBarrierWithGroupSyncintrinsic.It should be noted that the test cases are sized such that they are required to be evaluated over multiple waves. In this regard, each scenario fails using WARP without the barriers. Observing a failure is dependent on the scheduling of waves.
Resolves: #142.