Skip to content
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

Added unused output target warning and moved usage mask fill location #3947

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

gracejennings
Copy link
Contributor

Added a warning for output targets that are not fully written in a shader by checking the signature element usage mask.

Also moved the location where the signature element usage masks are filled to more accurately reflect which channels have been assigned for each element. The signature element usage masks were previously completely filled for a target if at least one of the channels were assigned.

Added unused output target warning and test
Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No functional suggestions. just some things that might improve readability.

As to the test failure, it's falling prey to the devops issue with macos-10 and python3. It seems to be pulling the yaml file from your local branch instead of the main branch. If you rebase your local branch and push the change here, I think the test will pass

: SV_Target0, out float4 outRT1
: SV_Target1, out float4 outRT2
: SV_Target2, out float4 outRT3
: SV_Target3) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this fell victim to some autoformatting that makes it confusing to read. I think it's worth changing it so that the SV_Target*s are on the same line as the parameters they apply to.

DxilSignature &sig = DM.GetOutputSignature();
for (auto &Elt : sig.GetElements()) {
if (Elt->GetKind() == Semantic::Kind::Target) {
if (Elt->GetUsageMask() != Elt->GetColsAsMask()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might consider combining these conditionals with && to reduce the indenting, which for me hinders readability

@AppVeyorBot
Copy link

Copy link
Member

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@AppVeyorBot
Copy link

@gracejennings gracejennings merged commit 5fba0c3 into master Sep 14, 2021
@gracejennings gracejennings deleted the user/gjennings/target_warning branch October 20, 2021 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants