-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lgc: kill unused outputs based on channelWriteMask #2774
Conversation
Test summary for commit 4340863CTS tests (Failed: 0/138994)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is mostly nice, but I think the code in handleColorExportInstruction
should be cleaned up while we're making this change. You can tell that there's a weakness because the remapping for EXP_FORMAT_32_AR needs to be addressed in two different places, and the logic for compressed exports is kind of difficult to follow.
How about the following:
comps
currently has two different meanings. It is initially used to represent the components written by the application shader. It is then later used to represent the data to be exported. This overload of meanings should be avoided by using two different arrays; perhaps call themcomps
andexports
. (They can bestd::array<Value *, 4>
instead ofSmallVector
)- Initially
exports
with poison values and then fill in the real data as well asexportMask
in a single pass within each of the cases of the switch statement based on the export format (taking the compCount and channelWriteMask into account) - Also, there's currently a duplicate export creation for the non-half case and the half-but-gfx11 case. (Not from your change but pre-existing.) Please try to avoid that.
Thank you for the detailed comments. I will update the code as you suggestions. |
V1: Refine the logic in |
Test summary for commit 03c7675CTS tests (Failed: 526/138995)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
Test summary for commit 03c7675CTS tests (Failed: 279/138995)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
Test summary for commit 03c7675CTS tests (Failed: 414/208498)
Rhel 8.9, Gfx10Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this already looks much better. A few remaining comments inline, and looks like there's still a bug hidden somewhere.
V2: Use Intrinsic ID. |
Test summary for commit bc2cb88CTS tests (Failed: 95/137948)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
Test summary for commit dc096a2CTS tests (Failed: 0/137948)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
This is an optimization of output caculations based on channelWriteMask of the pipeline stats from two aspects: - Remove export instructions for color targets that have a 0 write mask - Replace components that aren't used according to the write mask with 'poison'
V3: update a lit test. |
Test summary for commit e8d466aCTS tests (Failed: 0/137948)
Ubuntu navi3x, SrdcvkUbuntu navi2x, Srdcvk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This is an optimization of output calculations based on channelWriteMask of the pipeline stats from two aspects:
(cherry picked from commit 4dd226794ec1a09b3b95063197f1a4842145f548)