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
added 2 commits
June 5, 2025 14:41
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
added 2 commits
June 16, 2025 15:57
I guess only MSVC doesn't have uint alias? Unintended anyway.
bob80905
reviewed
Jun 17, 2025
tex3d
reviewed
Jun 17, 2025
tools/clang/test/CodeGenDXIL/hlsl/intrinsics/buffer-agg-load-stores.hlsl
Outdated
Show resolved
Hide resolved
added 2 commits
June 23, 2025 16:26
These might have confused the checks for multi stores where relevant. By adding the checks, the number of stores must be as expected and there will be no missed failures.
Changed the usage to logical && to skip the immediate conversion usage which wasn't that distinct.
tex3d
reviewed
Jun 24, 2025
tools/clang/test/CodeGenDXIL/hlsl/intrinsics/buffer-agg-load-stores.hlsl
Outdated
Show resolved
Hide resolved
tex3d
approved these changes
Jun 25, 2025
V-FEXrt
approved these changes
Jun 25, 2025
pow2clk
pushed a commit
to pow2clk/DirectXShaderCompiler
that referenced
this pull request
Jun 25, 2025
…ft#7513) 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 microsoft#7508 (cherry picked from commit e07be1c)
pow2clk
pushed a commit
that referenced
this pull request
Jun 25, 2025
Cherry-picked to release-1.8.2505 from #7513 There were two problems with processing the status parameter with the rework 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 (cherry picked from commit e07be1c)
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