Add base and core bindless validation instrumentation classes#2014
Add base and core bindless validation instrumentation classes#2014s-perron merged 22 commits intoKhronosGroup:masterfrom
Conversation
|
Getting id shift in my tests on the linux-based platforms. Windows is passing. Any ideas? |
Specifically, these are functions which call TakeNextId(). These need to be called in a specific order to guarantee that tests which do exact compares will work across all platforms. c++ pretty much does not guarantee order of evaluation of operands, so any such functions need to be called separately in individual statements to guarantee order.
|
Figured out the id shift problem. I had two different routines that were doing TakeNextId() and was calling them as arguments to a function call where, I presume, the order of execution is not guaranteed across platforms. |
include/spirv-tools/instrument.hpp
Outdated
| // Error Codes | ||
| // These are utilized in debug output record streams and identify which | ||
| // validation error has occurred and written a specific record. | ||
| static const uint32_t kInstErrorBindlessBounds = 0; |
There was a problem hiding this comment.
Do all of these need to be in the public api? If not, this should probably live in the opt/ directory and these should be enum classes or something.
There was a problem hiding this comment.
Yes, these are all intended to be exported outside of spirv-opt. They will be used by the Vulkan bindless validation layer to read the data written by the instrumented bindless code and pass it to the Vulkan developer to understand what error occurred and where. Is there a better place for this file?
There was a problem hiding this comment.
In fact, I had a request to add a few more, which I am going to do next.
There was a problem hiding this comment.
Is there a bug or design doc I can look at to better understand how this works/ what it does?
There was a problem hiding this comment.
Basically, this commit creates the pass InstBindlessCheckPass. It's purpose is documented in optimizer.hpp at CreateInstBindlessCheckPass() and in inst_bindless_check_pass.h. A supporting class InstrumentPass is also created and is documented in instrument_pass.h. This documentation should go a ways in giving the context for the objects in instrument.hpp.
Nonetheless, I agree that some additional documentation in instrument.hpp would be helpful, so I will try to add some.
There was a problem hiding this comment.
I have now updated instrument.hpp to make it friendlier to external users/developers. Let me know if you have additional suggestions.
|
Building of the validation files is failing although I haven't made any (direct) changes in the area. Any thoughts on what is going on here or how to proceed? |
|
Looks like it failed on windows due to PDB issues. Usually re-building fixes the problem. |
|
Related to what @dj2 was asking for, now that we have the official PR, is there a spec or something in gitlab that describes how all of this is suppose to work with all of the other parts? What tasks will be validation layer be responsible for, and how will a user know which descriptor set and bindings the output will be located in? Are these error codes defined in a specification for the feature? What about the layouts for the messages? I would like to know that the external interface is agreed upon and what it is so I have a way of verifying that it is implemented correctly. Also, should some of the things exist in spirv-headers, or something like that. If they are in spirv-tools, any project that uses this feature will have to depend on spirv-tools. |
InstBindlessCheckPass is designed to partially address gitlab issue 1327 (Add Bounds Checking to Layers Using GPU-Assisted Validation) and InstrumentPass is designed to partially address 1293 (Develop tooling for shader instrumentation?). These are the only public documents for this work. The most recent design in 1327 should give you some context to understand the larger layer mechanism and how the instrumentation fits into it. There is a lower-level design document that the layer developer is keeping here internal to LunarG, but as internal document, it is not clear how useful it would be for you to verify against it. You should note that I have documented in inst_bindless_check_pass.h that the external design of InstBindlessCheckPass is wholly intended to address the needs of the bindless validation layer and that its external design may change as the needs of that layer change. This is meant to give a heads up to anyone else who builds anything against this design. That said, the design of both InstBindlessCheckPass and the layer are pretty much set, at least for the core functionality. We have a version of the layer that was recently demo'd at the last F2F and is running solidly on at least one real game. So I am not expecting too much more change, which is why I am now attempting to check this in. Can we just assume that the functionality that I document in inst_bindless_check_pass.h, instrument.hpp and optimizer.hpp is useful to the community, and verify that the code matches the documentation?
As is documented in optimizer.hpp, the descriptor set is passed in by the layer through CreateInstBindlessCheckPass(). As is documented in instrument.hpp, the binding for the stream output buffer is 0. Let me know if you think this should be documented some other way.
The documentation I have written is for the developer of the validation layer. The developer of the validation layer will decide what and how to document to the user of the layer since that is really a separate design.
The external interface for the output buffer for InstBindlessCheckPass is essentially what is documented in instrument.hpp. The layer includes instrument.hpp and uses those static consts, so by definition there is agreement on the external interface.
If the layer wishes to publish offsets somewhere to allow other tools to instrument to the layer, that would be their prerogative, but for now the layer is giving the control of the format of the output buffer to InstBindlessCheckPass. |
|
This is documented elsewhere, but I thought I would point out that this current commit only covers core bindless functionality. The core functionality assumes that descriptor arrays are of compile-time constant size and all descriptors are initialized. Future commits will cover the vk_ext_descriptor_indexing extension, which allows for runtime-sized descriptor arrays and uninitialized descriptors (if unused). My current thinking is that I will extend InstBindlessCheckPass to handle runtime-sized descriptor arrays, but will create a separate pass to check for uninitialized descriptors. Both passes will use the same output buffer stream to record errors. |
s-perron
left a comment
There was a problem hiding this comment.
A couple changes. I still want to to discuss the location for the header file instrument.hpp.
|
We talked internally about the header file, and it is fine. I don't really like |
My view was that these constants are tightly coupled to the code that does the instrumentation. The new instrumentation code is being added to SPIRV-Tools so it's natural to put this header here. It's good that the new constants are in their own header file, so it's clear that it's a different part of the API surface. In future, we might expose the optimizer's classes and APIs for representing, manipulating, and analyzing a SPIR-V module (a real representation for update purposes). In that case this instrumentation code could take a life of its own in its own repository (and independent maintenance, evolution, and leadership). In that case the code doing the instrumentation and its interface header would move to that other repository. That future is still open to us, and that's great. |
Roll third_party/glslang/ 0de87ee..6c47979 (4 commits) KhronosGroup/glslang@0de87ee...6c47979 $ git log 0de87ee..6c47979 --date=short --no-merges --format='%ad %ae %s' 2019-12-09 cepheus Fix KhronosGroup#2020: PR KhronosGroup#1977 broke HLSL member consistency, this finishes it... 2019-12-09 cepheus Fix: KhronosGroup#2014: Don't do "extension-on && version >= ..." keyword checks. 2019-12-09 cepheus Fix KhronosGroup#2007: Fix a couple relative header paths in header files. 2019-12-09 cepheus Fix KhronosGroup#1993: Fully exclude ftransform() from SPIR-V semantics. Roll third_party/googletest/ ae8d1fc81..78fdd6c00 (6 commits) google/googletest@ae8d1fc...78fdd6c $ git log ae8d1fc81..78fdd6c00 --date=short --no-merges --format='%ad %ae %s' 2019-12-05 absl-team Googletest export 2019-12-05 absl-team Googletest export 2019-12-05 absl-team Googletest export 2019-11-27 krystian.kuzniarek Revert "remove MSVC workaround: wmain link error in the static library" 2019-11-27 krystian.kuzniarek Revert "unify googletest and googlemock main functions" 2019-11-17 krystian.kuzniarek remove MSVC workaround: cease const dropping Roll third_party/re2/ bb8e77755..6a86f6b3f (1 commit) google/re2@bb8e777...6a86f6b $ git log bb8e77755..6a86f6b3f --date=short --no-merges --format='%ad %ae %s' 2019-12-09 junyer Simplify the bytecode for the 80-10FFFF rune range. Roll third_party/spirv-cross/ 15b860eb1..f912c3289 (2 commits) KhronosGroup/SPIRV-Cross@15b860e...f912c32 $ git log 15b860eb1..f912c3289 --date=short --no-merges --format='%ad %ae %s' 2019-12-10 post GLSL: Fix array of input patch variables. 2019-12-09 post GLSL: Fix EmitStreamVertex/Primitive. Roll third_party/spirv-tools/ e82a428..0a2b38d (2 commits) KhronosGroup/SPIRV-Tools@e82a428...0a2b38d $ git log e82a428..0a2b38d --date=short --no-merges --format='%ad %ae %s' 2019-12-10 afdx spirv-fuzz: function outlining fuzzer pass (KhronosGroup#3078) 2019-12-06 afdx spirv-fuzz: Use validator to check break/continue dominance conditions (KhronosGroup#3089) Created with: roll-dep third_party/effcee third_party/glslang third_party/googletest third_party/re2 third_party/spirv-cross third_party/spirv-headers third_party/spirv-tools
No description provided.