Skip to content

Implement SPV_KHR_ray_query generation#2834

Merged
ehsannas merged 5 commits into
microsoft:masterfrom
jiaolu:master
May 11, 2020
Merged

Implement SPV_KHR_ray_query generation#2834
ehsannas merged 5 commits into
microsoft:masterfrom
jiaolu:master

Conversation

@jiaolu
Copy link
Copy Markdown
Contributor

@jiaolu jiaolu commented Apr 17, 2020

from dxr 1.1 rayquery hlsl

from dxr 1.1 rayquery hlsl
@msftclas
Copy link
Copy Markdown

msftclas commented Apr 17, 2020

CLA assistant check
All CLA requirements met.

@ehsannas ehsannas self-requested a review April 17, 2020 14:21
@ehsannas ehsannas added the spirv Work related to SPIR-V label Apr 17, 2020
@ehsannas
Copy link
Copy Markdown
Contributor

@jiaolu Please check the test failures.

@AppVeyorBot
Copy link
Copy Markdown

@jiaolu
Copy link
Copy Markdown
Contributor Author

jiaolu commented Apr 17, 2020

Please check the test failures.
@ehsannas Is there a way to run single failed test? .e.g . my one failed test : WholeFileTest.PassThruPixelShader

@ehsannas
Copy link
Copy Markdown
Contributor

run

clang-spirv-tests.exe --spirv-test-root C:\DirectXShaderCompiler\tools\clang\test\CodeGenSPIRV --gtest_filter= WholeFileTest.PassThruPixelShader

@AppVeyorBot
Copy link
Copy Markdown

@jiaolu
Copy link
Copy Markdown
Contributor Author

jiaolu commented Apr 20, 2020

Please check the test failures.
@ehsannas, The checks have passed.

@jiaolu
Copy link
Copy Markdown
Contributor Author

jiaolu commented Apr 23, 2020

@ehsannas, Any comment on this review?

@ehsannas
Copy link
Copy Markdown
Contributor

Sorry, I'm falling behind in code reviews. I'll definitely review this as soon as I can.

@ehsannas ehsannas self-assigned this Apr 24, 2020
Copy link
Copy Markdown
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.

Thanks for your contribution. In general this is a lot of good work. Just a few comments:

  1. Please run clang-format on the files you have changed. (if you're using VS or VSCode, you can install clang-format or ClangFormat extensions that make it easy to do this).

  2. The .rst file (the documentation file) is not formatted correctly, so github can't render it.
    See your file vs original file

  3. Have you verified that the generated SPIR-V works on hardware? I don't have access to a device that can perform these operations.

  4. This extension is currently provisional. the spec says

This extension is provisional and should not be used in production applications. The functionality may change in ways that break backwards compatibility between revisions, and before final release.

Do you recommend waiting for the extension to become final first?

Comment thread docs/SPIR-V.rst
Comment thread docs/SPIR-V.rst Outdated
@dgkoch
Copy link
Copy Markdown

dgkoch commented May 6, 2020

Do you recommend waiting for the extension to become final first?

We'd like to have this available before final so that people can experiment with it.
Obviously it'll need to be updated to match final when that is available.

@ehsannas
Copy link
Copy Markdown
Contributor

ehsannas commented May 6, 2020

We'd like to have this available before final so that people can experiment with it.
Obviously it'll need to be updated to match final when that is available.

@dgkoch Sounds good to me.

@jiaolu Please add the following warning to the beginning of the two functions processTraceRayInline and processRayQueryIntrinsics:

emitWarning("SPV_KHR_ray_query is currently a provisional extension and might
 change in ways that are not backwards compatible", expr->getExprLoc());

@jiaolu
Copy link
Copy Markdown
Contributor Author

jiaolu commented May 7, 2020

code updated.

@AppVeyorBot
Copy link
Copy Markdown

@jiaolu
Copy link
Copy Markdown
Contributor Author

jiaolu commented May 9, 2020

yes, i have tested dxc output of rayquery spv in our internal setup environment.

@jiaolu
Copy link
Copy Markdown
Contributor Author

jiaolu commented May 9, 2020

@ehsannas, Any more comments about this review?

@ehsannas
Copy link
Copy Markdown
Contributor

Thanks @jiaolu . I have reverted all the changes to HlslTypes.cpp because it was just spacing changes and it was distracting from the main goal of this PR (sadly some files within DXC currently don't adhere to clang-format). The formatting of the doc file was also still incorrect which resulted in Github not being able to render the file correctly. I've fixed those too.

@jiaolu
Copy link
Copy Markdown
Contributor Author

jiaolu commented May 11, 2020

@ehsannas , Thanks for the fix

@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.

5 participants