Validate presence of Stride operand to OpCooperativeMatrix{Load,Store}KHR #5777
Conversation
The specification requires the Stride operand for the RowMajorKHR and ColumnMajorKHR layouts. Signed-off-by: Kevin Petit <kevin.petit@arm.com> Change-Id: I51084b9b8dedebf9cab7ae25334ee56b75ef0126
There was a problem hiding this comment.
When I was re-reading the extension it struck me that we don't explicitly say how to interpret stride. I agree with the implication here that stride should interpreted as unsigned, but there is an ambiguity in my mind in the spec arising from the description of the memory layouts:
... OpCooperativeMatrixLoadKHR or OpCooperativeMatrixStoreKHR. Stride must be greater than 0 when passed to OpCooperativeMatrixStoreKHR and must be greater than or equal to 0 when passed to OpCooperativeMatrixLoadKHR.
If it is unsigned why say greater or equal to 0? Might be worth a clarification to the spec.
Co-authored-by: alan-baker <alanbaker@google.com>
Change-Id: I06d7308c4a2b62d26d69e88e03bfa009a7f8fff3
|
Thanks for the review! Agree the spec could be improved. I've created KhronosGroup/SPIRV-Registry#279 to discuss spec clarifications. If you're fine with deferring further improvements to the validation of the Stride operand to a separate change (my intent was only to catch the absence of it when required with this change) then this change should now be ready for another review. If you think we should do it all in this PR, then we'll need to wait for KhronosGroup/SPIRV-Registry#279 to be resolved. |
|
I ran this against the CTS cooperative matrix tests, and didn't have any issues. |
Change-Id: I9cbabec0ed2172dcd228cc385551cb7a5b79df1a
|
The shaderc failure appears entirely unrelated to this change. |
…}KHR (KhronosGroup#5777) * Validate Stride operand to OpCooperativeMatrix{Load,Store}KHR The specification requires the Stride operand for the RowMajorKHR and ColumnMajorKHR layouts. Signed-off-by: Kevin Petit <kevin.petit@arm.com> Change-Id: I51084b9b8dedebf9cab7ae25334ee56b75ef0126 * Update source/val/validate_memory.cpp Co-authored-by: alan-baker <alanbaker@google.com> * add test to exercise memory layout from spec constant and fix validation Change-Id: I06d7308c4a2b62d26d69e88e03bfa009a7f8fff3 * format fixes Change-Id: I9cbabec0ed2172dcd228cc385551cb7a5b79df1a --------- Signed-off-by: Kevin Petit <kevin.petit@arm.com> Co-authored-by: alan-baker <alanbaker@google.com>
The specification requires the Stride operand for the RowMajorKHR and ColumnMajorKHR layouts.
Change-Id: I51084b9b8dedebf9cab7ae25334ee56b75ef0126