Add support to convert DXR HLSL to SPV_NV_ray_tracing#1920
Add support to convert DXR HLSL to SPV_NV_ray_tracing#1920ehsannas merged 18 commits intomicrosoft:masterfrom
Conversation
|
✅ Build DirectXShaderCompiler 1.0.1317 completed (commit 519c89f944 by @alelenv) |
|
@alelenv : Thanks for this contribution! For better inclusiveness, I'd prefer to see a copy of the HLSL functional spec in a more plain-text format, for better inclusiveness. Yes the .docx renders apparently correctly on my Mac, but it seems to be straining to do so. I'm also a little concerned that the reference is on someone's personal .org page, and could disappear at a whim. Ideally a copy would be checked in with this PR (in the docs subtree). However, that doc lacks a copyright or author notice, so ... check with the author? I don't mean to come off as overly nit-picky, but I'd like this to be a stable documented feature of the compiler. |
5c5b9a7 to
549193c
Compare
b9bb79b to
b5dcf5c
Compare
|
✅ Build DirectXShaderCompiler 1.0.1324 completed (commit 307303f3c8 by @alelenv) |
There was a problem hiding this comment.
Hi @alelenv
Thanks for your contribution. It's great to see this work. The code looks good overall, but needs some improvements.
You'll find that many of my comments are related to adhering to the C++ style guide.
-
The style guide requires proper Punctuation, Spelling and Grammar. For example, comments should be proper sentences ending with a period. Consistently use
SPIR-V, rather thanspirvorspir-vorSpir-v, etc in the comments. I know this sounds nit-picky, but it'll lead to a much nicer code base over time (as it has happened over the development period of Spiregg). -
clang-format should be applied to all the files that you have touched. (you can use this extension if you're using VisualStudio)
-
I'm very glad to see you have added new tests \o/ These will prevent regressions in RT code generation in the future. Please add more if you can.
Here are a few that I thought might be useful:
What happens if more than 1 function has the same [shader(X)] attribute? Add a test for it.
Add a test that exercises the matrix transpose logic inprocessRayBuiltins.
etc.
Taking testing one step further: Would you be able to test the resulting SPIR-V on hardware to ensure correctness? I don't have access to the GPU that has the RT features. That would be the ultimate test for your work. -
SPIR-V.rst needs to be updated (we'll discuss this more on the next round of code-review).
-
Please do not force push to your branch when addressing code-review comments. Please make new commits, so that we can review the diff more easily.
-
I'll do (at least one) more rounds of code-review and testing before landing.
Thanks!
There was a problem hiding this comment.
Use
/// \brief Creates SPIR-V instructions for NV raytracing ops.
There was a problem hiding this comment.
/// These include the following SPIR-V opcodes:
There was a problem hiding this comment.
This is very useful. Thanks for creating the custom matrix QualType 👍
There was a problem hiding this comment.
the { should be on the previous line.
It looks like you haven't run clang-format ? If not, please do. Thanks.
There was a problem hiding this comment.
In general static member variables are forbidden due to their lifetime and static initialization issues.
static functions are OK.
So, there are a couple of different ways you can address this:
- remove these static data members. Have the static getter functions construct an
ExecutionModelobject and return it. e.g.
static ExecutionModel GetByStageName(llvm::StringRef stageName);
static ExecutionModel GetByShaderKind(ShaderKind sk);there'll be switch statements inside these functions I'd presume.
- Another way -- if we really really want to have only 14 objects and reuse them -- would be to use
SpirvContext. The context class keeps all the objects that live for the lifetime of the backend. Given how lightweight the objects of this class are, I don't think it's necessary to go down this route.
There was a problem hiding this comment.
Can this class be fully replaced with a standalone function (in SpirvEmitter.cpp) that returns the SPIR-V execution model for a given shader model kind?
e.g.
spv::ExecutionModel getSpirvExecutionModel(hlsl::ShaderModel::Kind smk);There was a problem hiding this comment.
All these functions are reimplementing the logic in DxilShaderModel.h.
I wonder if we could just use the hlsl::ShaderModel instead of the ExecutionModel throughout the spiregg code, and remove ExecutionModel class completely? Is there a good reason for this class to exist?
There was a problem hiding this comment.
We can't use hlsl::ShaderModel because there are no ShaderModel objects for RT profiles in DxilShaderModel.cpp (rgen_6_4, miss_6_4, etc.).
So I implemented a similar class (ExecutionModel) which would allow me to track individual RT shader stages.
Nonetheless, I have removed ExecutionModel class and used ShaderModel::Kind to track current entry point.
Let me know if this change works for you.
tools/clang/lib/SPIRV/SpirvEmitter.h
Outdated
There was a problem hiding this comment.
you can remove this constructor.
There was a problem hiding this comment.
As you may have noticed, we don't perform new memory allocation anywhere except in SpirvContext (which uses placement new) which is responsible for managing all allocated memory and releasing them.
I suggest you change your data structure from
llvm::DenseMap<const DeclaratorDecl *, const FunctionInfo *> functionInfoMap;to
llvm::DenseMap<const DeclaratorDecl *, FunctionInfo> functionInfoMap;and then
functionInfoMap[funcDecl] = FunctionInfo(...);
workQueue.insert(&functionInfoMap[funcDecl]);... hmmm... it looks like you can further simplify this:
Remove functionInfoMap and change
llvm::SetVector<const DeclaratorDecl *> workQueue;to
llvm::MapVector<const DeclaratorDecl *, FunctionInfo> workQueue;this way, workQueue[funcDecl] is the same as what funcMapInfo[funcDecl] used to be ; and if I understand llvm::MapVector correctly, you can iterate over it the same as as SetVector, so you can still have the loop over workQueue.
There was a problem hiding this comment.
@sparmarNV
I'm doing further testing on this PR, and I'm running into some failures. They are due to this suggestion that I made:
functionInfoMap[funcDecl] = FunctionInfo(...);
workQueue.insert(&functionInfoMap[funcDecl]);It turns out llvm::DenseMap may choose to overwrite that address (&functionInfoMap[x]) when handling its buckets. Sorry about that. I have made the fix, and I'll post my fix to this PR directly. I have created a wrapper function that updates functionInfoMap and workQueue at the same time, and it uses placement new to allocate memory from the SpirvContext memory pool.
There was a problem hiding this comment.
Thanks for testing our changes and I am glad that you were able to triage and revert this change before the merge.
|
@dneto0 : Sure. We are working on providing you a text copy soon which we can check into docs. @ehsannas : Thanks for such a detailed review. Q) Add a test that exercises the matrix transpose logic in Q) Taking testing one step further: Would you be able to test the resulting SPIR-V on hardware to ensure correctness? I don't have access to the GPU that has the RT features. That would be the ultimate test for your work. |
|
✅ Build DirectXShaderCompiler 1.0.1331 completed (commit 6e257b7d22 by @alelenv) |
Cool! I couldn't find
Awesome! Glad to hear that. |
Update tests to add multple entry functions of same shader stage
|
HLSL Spec is provided by Microsoft to users registered to DX forums I have updated link above to publicly available HLSL specs from MSDN |
|
✅ Build DirectXShaderCompiler 1.0.1352 completed (commit 91ed87a3f5 by @alelenv) |
- This change removes ExecutionModel class and relies on ShaderModel::Kind to track current entry point shader stage - Also instead of declaring it in SpirvEmitter, DeclResultIdMapper & GlPerVertex, we declare it only once in common object SpirvContext
ehsannas
left a comment
There was a problem hiding this comment.
Thanks for making this change 👍 . This is much nicer than what was there before.
You have eliminated a whole class, eliminated the use of static members, eliminated copies of ExecutionModel in different places and concentrated it all in SpirvContext. All great changes 👍 .
Please run clang-format if you haven't already.
| declIdMapper.setSpvExecutionModel(curEntryOrCallee->spvExecModel); | ||
| spvExecModel = curEntryOrCallee->spvExecModel; | ||
| entryFunction = curEntryOrCallee->entryFunction; | ||
| spvContext.setCurrentShaderModelKind(curEntryOrCallee->shaderModelKind); |
There was a problem hiding this comment.
Great to see the 3 lines is replaced with 1 line. Shader kind is now only stored in 1 place (SpirvContext), and this makes the code less error-prone. 👍
There was a problem hiding this comment.
use uint32_t instead of unsigned to be consistent with the rest of the code.
There was a problem hiding this comment.
I see both unsigned and uint32_t being used in the file.
Nonetheless, I will update the type to uint32_t.
There was a problem hiding this comment.
Looks like a few unsigned have snuck through in the past. But let's strive to stay consistent. I'll update the other ones to also use uint32_t in a different PR. Thanks for updating.
There was a problem hiding this comment.
Happy to see this is gone.
| inline bool isInputStorageClass(const StageVar &v); | ||
|
|
||
| private: | ||
| const hlsl::ShaderModel &shaderModel; |
There was a problem hiding this comment.
Happy to see this is gone.
There was a problem hiding this comment.
Happy to see this is gone.
There was a problem hiding this comment.
I believe new should be avoided here. See my comment about workQueue
There was a problem hiding this comment.
Yes, I am working on this next. Thanks for the review 👍
There was a problem hiding this comment.
I tried replacing functionInfoMap and workQueue with a MapVector, but it seems MapVector has few limitations.
We can't access the vector elements inside the MapVector with an index, which doesn't work in below case -
// The queue can grow in the meanwhile; so need to keep evaluating
// workQueue.size().
for (uint32_t i = 0; i < workQueue.size(); ++i) {
const FunctionInfo *curEntryOrCallee = workQueue[i];
...
}
Also using iterator on MapVector doesn't work with growing queue.
So I have gone with your option 1, which is to remove 'new' memory allocation by changing the DenseMap decl to -
llvm::DenseMap<const DeclaratorDecl *, FunctionInfo> functionInfoMap;
There was a problem hiding this comment.
@sparmarNV Ah you're right. It is not possible to iterate over a MapVector when it is growing. Thanks for removing the usage of new.
|
|
||
| // Even though the 'workQueue' grows due to the above loop, the first | ||
| // 'numEntryPoints' entries in the 'workQueue' are the ones with the HLSL | ||
| // 'shader' attribute, and must therefore be entry functions. |
There was a problem hiding this comment.
I believe new should be avoided here. See my comment about workQueue
tools/clang/lib/SPIRV/SpirvEmitter.h
Outdated
There was a problem hiding this comment.
No need for SpirvEmitter::
static hlsl::ShaderModel::Kind getShaderModelKind(StringRef stageName);This is caught by buildbots on Linux and macOS
Same for getSpirvShaderStage below.
There was a problem hiding this comment.
Happy to see that the class is gone. Especially the static data that was included in it.
|
✅ Build DirectXShaderCompiler 1.0.1390 completed (commit e03243ece5 by @alelenv) |
This change also - - removes invalid "SpirvEmitter::" from function declarations in SpirvEmitter class. - fix build errors by adding a default constructor in FunctionInfo struct to allow functionInfoMap allocate an empty object for no search results.
|
@ehsannas : I believe we have completed updates based on your feedback for first round of reviews |
ehsannas
left a comment
There was a problem hiding this comment.
@alelenv Thanks for addressing my comments so far. If you go through all the files, there are a few (mostly formatting) ones left to fix.
We're almost there.
Regarding documentation:
We have a comprehensive doc file about mapping HLSL concepts to SPIR-V. The file (SPIR-V.rst) is located here. You should update this file to reflect the changes you have made in this PR.
I think it'd be good to add a top-level section about Ray Tracing between Shader Model 6.0 Wave Intrinsics and Supported Command-line Options sections. And in that Ray Tracing section, feel free to add as many sub-sections as you need. The goal is to highlight the the RTX concepts/features and how they map from HLSL to SPIR-V. In other sections of this document we've tried to provide simple explanations for people to get started, and provide external links for full details. For example, you can also provide a link to the RTX HLSL spec, and the SPIR-V NV RT extension spec. It'd also be really nice to provide a link to sample shaders (e.g. raytracing helloworld shaders) that people can use to get started.
Under HLSL Shader Stages, you should add a Ray Tracing Stages subsection, and add the newly supported shader stages under that. For each stage, it'd be good to provide some explanation of the stage and also list the expected shader inputs and outputs (stage variables). Including Figure 1 and Figure 5 from @nsubtil's blog post could be very useful here.
In general I found that blog post to be quite a good introduction. You could use some of its content as brief explanations for the new documentation you are adding to SPIR-V.rst.
Thanks!
There was a problem hiding this comment.
revert to previous format (clang-format).
tools/clang/lib/SPIRV/SpirvEmitter.h
Outdated
| SpirvInstruction *retVal = | ||
| declIdMapper.getBuiltinVar(builtin, builtinType, callExpr->getExprLoc()); | ||
| retVal = spvBuilder.createLoad(builtinType, retVal); | ||
| if (transposeMatrix) |
There was a problem hiding this comment.
This is done now. Thanks.
| builtin = spv::BuiltIn::WorldToObjectNV; | ||
| break; | ||
| default: | ||
| emitError("ray intrinsic function unimplemented", callExpr->getExprLoc()); |
There was a problem hiding this comment.
Should we return nullptr after emitError?
|
✅ Build DirectXShaderCompiler 1.0.1400 completed (commit 2d8ad29f11 by @alelenv) |
|
✅ Build DirectXShaderCompiler 1.0.1401 completed (commit 76ae56e1e5 by @alelenv) |
|
@alelenv @sparmarNV Thanks for your updates. |
|
✅ Build DirectXShaderCompiler 1.0.1403 completed (commit a5ead487d5 by @alelenv) |
Also bundle the insertion into functionInfoMap and workQueue together.
|
@ehsannas : Sorry for missing a large number of formatting fixes from previous comments. My tracking changes for each comment via emails didnt work out properly. Thanks! |
|
✅ Build DirectXShaderCompiler 1.0.1413 completed (commit a0c65a70d6 by @alelenv) |
|
Just finished reading the documentation. Awesome job, @alelenv 👍. This is exactly what we were looking for. |
ehsannas
left a comment
There was a problem hiding this comment.
LGTM. Thanks for addressing all the code review comments \o/
I will do a bit of more testing, and if all goes well, I'll merge the PR (hopefully by tomorrow).
|
✅ Build DirectXShaderCompiler 1.0.1420 completed (commit 941395b8de by @alelenv) |
@ehsannas @antiagainst
Ehsan, Lei, please review the changes
Pattern match tests have been added for new features
Let us know if anything more is required from us
Thanks!