Skip to content

[spirv] Support SPV_EXT_demote_to_helper_invocation#2773

Merged
ehsannas merged 2 commits intomicrosoft:masterfrom
ehsannas:demote_to_helper
Mar 27, 2020
Merged

[spirv] Support SPV_EXT_demote_to_helper_invocation#2773
ehsannas merged 2 commits intomicrosoft:masterfrom
ehsannas:demote_to_helper

Conversation

@ehsannas
Copy link
Copy Markdown
Contributor

No description provided.

@ehsannas ehsannas added the spirv Work related to SPIR-V label Mar 18, 2020
@ehsannas ehsannas requested a review from jaebaek March 18, 2020 19:21
@ehsannas ehsannas self-assigned this Mar 18, 2020
@ehsannas ehsannas requested a review from s-perron March 18, 2020 19:46
@AppVeyorBot
Copy link
Copy Markdown

@ehsannas
Copy link
Copy Markdown
Contributor Author

The SPIR-V backend used to generate OpKill for the HLSL discard statement (reference). However, this is not quite correct. The proper behavior is to use OpDemoteToHelperInvocationEXT (reference)

@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

@jaebaek jaebaek left a comment

Choose a reason for hiding this comment

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

Mostly LGTM.
Just have 2 minor comments.

spvBuilder.setInsertPoint(newBB);

// The discard statement can only be called from a pixel shader
assert(spvContext.isPS());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am not sure what is the expected behavior from the AST parser, but I think we should print an explicit error message saying "The discard statement can only be called from a pixel shader".

I am just worried what if a user use release build and is not able to recognize it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done.

@@ -1,7 +1,9 @@
// Run: %dxc -T ps_6_0 -E main
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why don't we add a test that uses discard; in a vertex shader and check it with the expected error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@AppVeyorBot
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spirv Work related to SPIR-V

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants