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

SPIR-V: Emit RayTmaxKHR builtin for RayTCurrent IOP #3320

Merged
merged 2 commits into from
Dec 17, 2020

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented Dec 15, 2020

Fixes #3322

HitTKHR was removed from the spec as it merely shadows RayTmaxKHR. Emitting it results in invalid SPIR-V:

generated SPIR-V is invalid: Operand 3 of Decorate requires one of these capabilities: RayTracingNV
  OpDecorate %6 BuiltIn HitTNV

Replace the builtin by RayTmaxKHR if NV_ray_tracing is not enabled to resolve this.


Is this valid in all of the cases outlined here? I assume - given that that commit says it shadows RayTmax anyway, likely also on NV - that the max is updated upon hit?

HitTKHR was [removed] from the spec as it merely shadows RayTmaxKHR.
Emitting it results in invalid SPIR-V:

    generated SPIR-V is invalid: Operand 3 of Decorate requires one of these capabilities: RayTracingNV
      OpDecorate %6 BuiltIn HitTNV

Replace the builtin by RayTmaxKHR if NV_ray_tracing enabled to resolve
this.

[removed]: KhronosGroup/SPIRV-Headers@bdd2aa3
@AppVeyorBot
Copy link

@ehsannas ehsannas self-requested a review December 17, 2020 17:44
@ehsannas ehsannas added the spirv Work related to SPIR-V label Dec 17, 2020
@ehsannas
Copy link
Contributor

yeah I think it should cover everything

@ehsannas
Copy link
Contributor

More context:
HitTKHR has been removed as it is an alias for RayTMaxKHR. See: KhronosGroup/SPIRV-Headers@bdd2aa3

@ehsannas
Copy link
Contributor

@MarijnS95 would you be able to share a shader that reproduces the issue, so we can add it as a unit test? Or add a unit test to this PR? (adding a unit test is easy. See https://github.com/microsoft/DirectXShaderCompiler/pull/2512/files as an example). You can just have a // CHECK: for RayTmaxKHR

@MarijnS95
Copy link
Contributor Author

More context:
HitTKHR has been removed as it is an alias for RayTMaxKHR. See: KhronosGroup/SPIRV-Headers@bdd2aa3

@ehsannas Already in the main PR description as well as the topmost commit, including the link to that SPIRV-Headers change :)

@MarijnS95 would you be able to share a shader that reproduces the issue, so we can add it as a unit test? Or add a unit test to this PR? (adding a unit test is easy. See https://github.com/microsoft/DirectXShaderCompiler/pull/2512/files as an example). You can just have a // CHECK: for RayTmaxKHR

Done, writing tests is the easy part but realising disassembly prints these as RayTmaxNV (small m, NV instead of KHR) takes most of the time.
I didn't add tests to all possible (nv) shaders though, and KHR only has a closesthit test shader.

@ehsannas
Copy link
Contributor

Sounds good. Thanks

Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

LGTM. I'll merge once the bots are green.

@AppVeyorBot
Copy link

@ehsannas ehsannas merged commit f3ba78b into microsoft:master Dec 17, 2020
@MarijnS95 MarijnS95 deleted the hitt-khr branch December 17, 2020 23:38
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.

[SPIRV] Replacing HitTNV by RayTmaxNV
3 participants