-
Notifications
You must be signed in to change notification settings - Fork 567
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
Add ComputeDerivativeGroup*NV capabilities to trim capabilities pass. #5430
Conversation
No tests needed for this. The code path is well tested. Just adding new data.
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.
I think this is incomplete:
If neither derivative group mode was specified, the derivatives return zero.
Seems like removing the execution mode, but having the capability does have an effect: it allows the sampling instructions in GLCompute
.
So if such instruction is used in a compute shader, but the capability is removed, we'd produce the wrong module no?
I missed that statement in the spec. When I tried cases without the execution mode the validator complained:
|
In fact, that has been the behaviour since the validation for the extension was first implemented by Nvidia: bac82f4#diff-1232a89db063a58c947bb0e253a3161c55e2c5e49666aa96d55aaa6d59b0f8fdR78-R90 |
@dgkoch @jmacnak-nv Do either of you have any insight into the difference between the spec and the validator? |
@Keenuts I suggest we merge this, since this behaviour matches the validator, and we have not heard anything from NV. I have opened up a follow up issue. |
As long as the validator refuses to accept cases where we might remove the extension when required, that's fine for me. |
I'm looking at this now. I don't have any insight into the current validation support though, and the original author is no longer at NVIDIA. |
I agree that the validator seems be missing support for this, based on the above, and it does seem to contravene the spec. Not sure how important that functionality would be to keep, but it does mirror the language from the GLSL spec:
|
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.
LGTM for the change.
Now, it's up to you how to handle the validator issue.
Since both GLSL and the SPIR-V spec are consistent, maybe fixing the validator to correctly handle this case could be useful, but if this is required to unblock something, I'm fine with this temporary solution until it's rewritten with the validation rule.
No description provided.