Add test for CheckAccessFullyMapped #609
Conversation
| // Test that if CheckAccessFullyMapped is called on | ||
| // something other than an operation status, | ||
| // the LSB will be cast to a bool | ||
|
|
||
| unsigned int Nums[4] = {1, 2, 2147483647, 0}; | ||
| for (unsigned int I = 0; I < 4; I++){ | ||
| unsigned int Num_I = Nums[I]; | ||
| CAFM[idx] = CheckAccessFullyMapped(Num_I); | ||
| idx++; | ||
| } |
There was a problem hiding this comment.
I don't actually think we should test this usage - this feels like undefined behaviour territory. You aren't really allowed to call CheckAccessFullyMapped with anything other than the status value returned from a Sample, Gather, or Load: https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/checkaccessfullymapped - whatever it returns is kind of nonsense by definition.
There was a problem hiding this comment.
Note that there is a INSTR.CHECKACCESSFULLYMAPPED error code listed in https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/DXIL.rst, so I would have hoped that doing this would fail validation. I guess it doesn't though based on the test results?
|
This PR went the wrong direction, we shouldn't be testing that LLVM's implementation matches DXC's implementation of CheckAccessFullyMapped for incorrect usages. It's undefined behavior / something we don't need to worry about. The current test as it exists correctly tests the CheckAccessFullyMapped function, and the addition to the existing test is the wrong direction. |
2 participants
This PR adds a test for CheckAccessFullyMapped. It builds on the previous test that was added to test the partially mapped / reserved resources infrastructure.
The test can be extended and simultaneously used to test both the reserved resources infrastructure and CheckAccessFullyMapped, the former implicitly and the latter explicitly.
It also tests unsigned ints directly given to CheckAccessFullyMapped, validating results for non-status inputs.
Fixes #181
Clang is currently failing, but will pass once llvm/llvm-project#170949 goes in.