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]Support KHR_Ray_tracing terminate Ops #3295

Merged
merged 7 commits into from Dec 4, 2020

Conversation

jiaolu
Copy link
Contributor

@jiaolu jiaolu commented Dec 3, 2020

Add OpIgnoreIntersectionKHR/OpTerminateRayKHR related
#3285

@ehsannas ehsannas self-requested a review December 3, 2020 15:14
@ehsannas ehsannas added the spirv Work related to SPIR-V label Dec 3, 2020
@AppVeyorBot
Copy link

@ehsannas ehsannas requested a review from jaebaek December 3, 2020 22:21
@ehsannas
Copy link
Contributor

ehsannas commented Dec 3, 2020

@jiaolu @MarijnS95 @jaebaek Please review.

@AppVeyorBot
Copy link

Copy link
Contributor

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Thanks @ehsannas! Unfortunately this still seems to run into failed to optimize SPIR-V: OpLabel inside basic block

SourceLocation loc)
: SpirvTerminator(IK_RayTracingTerminate, opcode, loc) {
assert(opcode == spv::Op::OpTerminateRayKHR ||
spv::Op::OpIgnoreIntersectionKHR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
spv::Op::OpIgnoreIntersectionKHR);
opcode == spv::Op::OpIgnoreIntersectionKHR);

Praise clang for catching this one:

../tools/clang/lib/SPIRV/SpirvInstruction.cpp:994:47: warning: converting the enum constant to a boolean [-Wint-in-bool-context]
  assert(opcode == spv::Op::OpTerminateRayKHR ||
                                              ^
../tools/clang/lib/SPIRV/SpirvInstruction.cpp:995:10: error: value of type 'spv::Op' is not contextually convertible to 'bool'
         spv::Op::OpIgnoreIntersectionKHR);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Contributor

Choose a reason for hiding this comment

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

ouch. Sad that MSVC didn't catch this. Also not sure why Appveyor bot is passing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know why appveyor is passing... MSVC doesn't catch the build warning and the FileCheck tests run without optimizations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, we need to update SPIRV-Tools to recognize these instructions as terminator as well :) Will do so soon

Copy link
Contributor

@MarijnS95 MarijnS95 Dec 3, 2020

Choose a reason for hiding this comment

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

Another explanation would be Clang version. There are a few new warnings on Clang 11 but Appveyor is still at 10.0.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should land this PR, and we will separately make fixes to SPIRV-Tools for recognizing these instructions as termination instructions.

@jiaolu
Copy link
Contributor Author

jiaolu commented Dec 4, 2020

Look good to me.

@AppVeyorBot
Copy link

Copy link
Contributor

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Clang 11 build is green now, thanks!

@ehsannas ehsannas merged commit ce2ec2f into microsoft:master Dec 4, 2020
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.

None yet

5 participants