Refactor udt intrinsic arg copy to before SROA, flatten RayDesc#7440
Merged
tex3d merged 16 commits intomicrosoft:mainfrom May 16, 2025
Merged
Refactor udt intrinsic arg copy to before SROA, flatten RayDesc#7440tex3d merged 16 commits intomicrosoft:mainfrom
tex3d merged 16 commits intomicrosoft:mainfrom
Conversation
Intrinsics that take UDT arguments need copy-in/copy-out. Other aggregate args are flattened for intrinsic calls. Previously, these operations were intermingled, driven by SROA on alloca/GV values. There were RayDesc arguments that weren't treated consistently, leading to problems. They should be flattened into the intrinsic arguments, but TraceRay calls didn't do this. Doing this for TraceRay would cause an issue where the subsequent Payload UDT arg could be in a different position depending on whether it was processed before or after flattening the RayDesc. This change is in preparation for flattening RayDesc args. It separates the UDT copy-in/copy-out into a separate operation before SROA. When doing the copy-in/copy-out separately, we don't expand the memcpy operations until they are handled separately during SROA. This causes a change in some IR shape, which had to be adjusted for in some tests. There are a couple remaining PIX pass tests that still need adjustment. These will likely need further changes when flattening RayDesc, so maybe they should be revisited after that. TODO: - Fix the remaining tests - Flatten the RayDesc parameters - Adjust tests for accordingly
Contributor
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This renames: - copyIntrinsicUDTArgs -> copyIntrinsicAggArgs - RewriteCallArg -> memcpyAggCallArg. Updated to copy the RayDesc aggregate arguments as well. This will fix the issue with a RayDesc argument provided directly from a cbuffer, since the incoming argument pointer will no longer be skipped by SROA. Expected IR for tests will need to be updated after flattening RayDesc, so holding off on updating tests until then.
- Enable RayDesc flattening in SROA - Update lowering code accordingly - Separated MakeNop and MakeMiss lowering into two functions - Updated tests TODO: Update PIX pass tests
pow2clk
reviewed
May 12, 2025
Collaborator
pow2clk
left a comment
pow2clk
left a comment
There was a problem hiding this comment.
Looks sensible. Sorry if I was too detailed for a draft in some parts.
The PIX test or portion of test that relies on the old compiler behavior of alloca for RayDesc. Since these tests were made specifically to verfify this old pattern, it was ok to remove them.
tex3d
changed the title
pow2clk
approved these changes
May 14, 2025
Collaborator
pow2clk
left a comment
pow2clk
left a comment
There was a problem hiding this comment.
I don't have anything against this change, just some confirmations and very mild suggestions. Since you need a second review anyway, perhaps getting Jeff to comment on the pix changes would be expedient. I don't expect him to review the other parts though, so you might choose to get someone else to give those a glance as well.
pow2clk
approved these changes
May 15, 2025
Collaborator
pow2clk
left a comment
pow2clk
left a comment
There was a problem hiding this comment.
Approving again again again.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Intrinsics that take UDT arguments need copy-in/copy-out. Other aggregate args are flattened for intrinsic calls. Previously, these operations were intermingled, driven by SROA on alloca/GV values.
There were RayDesc arguments that weren't treated consistently, and weren't copied in when necessary, leading to problems. They should be flattened into the intrinsic arguments, but TraceRay calls didn't do this.
This change:
Fixes #7434.