Fix errors in retrieving and assigning load status parameter #7513
Conversation
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
The status Value was not getting passed all the way down to the load instruction generation for aggregate types because the helper constructor would always set it to null. It needs to be explicitly stated since by that point, the original call instruction it came from has been lost amidst subsequent GEPs, bitcasts, and/or loads that aggregate types (arrays and structs) will use on the results of the original call instruction to get the exact element required. This changes the constructor to take an optional status parameter allowing the locations where it might be set to pass it along. In other cases, it will be null and be appropriately ignored. Modified aggregate tests to verify this behavior. This required keeping track of the return of the last load operation involved in a raw buffer load, which made arrays more complicated. Rather than give them their own CHECK prefix, I lumped them in with large matrices requiring three loads. This did require making all the array lengths 3 to match. The loss in test variability is worth the convenience as there is no known distinction when it comes to array sizes over 1.
The status retrieval from the resret returned by the raw buffer loads was using the wrong index for native vectors supported by shader model 6.9. Adjusting the index by the opcode ensures that the index will be correct. This also required a change to validation that allows checkAccessFullyMapped to operate on the second element extracted from a resret where applicable and some corresponding null tolerance in related code. Adds status retrieving overloads to the relevant load/store tests. Fixes microsoft#7508
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 were two problems with processing the status parameter with the reword of the buffer load code. The first was that the status was not being passed down to the load instruction generation for aggregate types in any shader model version. The second was that the status retrieval from the resret returned by the raw buffer loads was using the wrong index for native vectors supported by shader model 6.9.
The status Value was not getting passed all the way down to the load instruction generation for aggregate types because the refactored helper constructor would always set it to null. It needs to be explicitly stated since by that point, the original call instruction it came from has been lost amidst subsequent GEPs, bitcasts, and/or loads that aggregate types (arrays and structs) will use on the results of the original call instruction to get the exact element required.
This changes the constructor to take an optional status parameter allowing the locations where it might be set to pass it along. In other cases, it will be null and be appropriately ignored.
Modified aggregate tests to verify this behavior. This required keeping track of the return of the last load operation involved in a raw buffer load, which made arrays more complicated. Rather than give them their own CHECK prefix, I lumped them in with large matrices requiring three loads. This did require making all the array lengths 3 to match. The loss in test variability is worth the convenience as there is no known distinction when it comes to array sizes over 1.
The status retrieval from the ResRet returned by the raw buffer loads was using the wrong index for native vectors supported by shader model 6.9. Adjusting the index according to the opcode ensures that the index will be correct.
This also required a change to validation that allows checkAccessFullyMapped to operate on the second element extracted from a ResRet where applicable and some corresponding null tolerance in related code.
Adds status retrieving overloads to the relevant load/store tests for sm6.9, aggregates, and other loads though the last category exhibited no issues. At least I got some statuses right!
Fixes #7508