-
Notifications
You must be signed in to change notification settings - Fork 694
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
Added implementation for ID3D12FunctionReflection1::GetDesc1 #6827
base: main
Are you sure you want to change the base?
Added implementation for ID3D12FunctionReflection1::GetDesc1 #6827
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
…ed some missing node information to the implementation.:
…_NODE_OVERRIDES_TYPE to ..._LAUNCH_TYPE
Hi - thank you for all your effort here! I just want to set appropriate expectations - this is a pretty large change adding a lot of new API surface area that we'll need to consider, taking into account future evolution of the language and compiler. As you may know we're hard at work adding HLSL support to clang, and so we need to consider the implications of a change that essentially exposes some amount of implementation details that may not be stable in the long term. Also, as we're focusing on clang, this means we have limited bandwidth for reviews of this nature, especially as we're in the middle of summer vacation season. So, be warned, it may take a while for us to find time to look at this properly. In the meantime, I suggest you have a look at CONTRIBUTING.md to make sure that you have all your ducks in a row ready for a successful review. For example, I think some tests may be required. Thanks again!
|
Sounds reasonable, though I'm not sure if you're referring to unit tests or that the tests are failing on this PR. |
I'm referring to unit tests with the code. From CONTRIBUTING.md:
|
…not really possible to change things in DirectX-Headers without a Windows SDK update... Removed stupid hacks in dxexp.cpp, which were there because d3d12.h comes from the windows SDK. Now that DirectX-Headers is required, d3d12.h comes from the DirectX-Headers, meaning that the structs & enums that were hardcoded are now not needed anymore. It can now happily use the same headers without relying on specific windows SDKs :)
Will take a look at Wednesday. Unexpectedly failing on remote. Also adding implementation for the reflection writer to know about these structs and output it as disassembly, along with a few unit tests. |
…com/Oxsomi/DirectXShaderCompiler into feat_query_all_function_properties3
…irectx headers have their own way of handling with it
…nted it in QueryInterface
…com/Oxsomi/DirectXShaderCompiler into feat_query_all_function_properties3
…on shaders numthreads to dxa.
…nition that DirectX-Headers uses to define GUIDs so that either implementation of __uuidof can find the GUID.
…e. Also don't like an include named winadapter.h in a file named WinAdapter.h
…rt. Added CLSID_D3D12SDKConfiguration for older Windows sdks
…iler into feat_query_all_function_properties3
…on info (where it makes sense). Only unit tests that provide unique scenarios have been touched. Fixed some misformatting in D3DReflectionDumper. Apparently m_pProps can be NULL, so made it S_OK instead of E_FAIL. Fixed problem with GetInputNode/GetOutputNode where it returned a string pointer of a C++ struct which implictly copied due to not being const&.
… else. Changed it to use ShaderReflection1 and LibraryReflection1
… in checks, same for ShaderReflection1. Also fixed the RDAT3 in wavesize-compute-node-rdat.hlsl
…s_as.hlsl is compiled as lib_6_8
This should now be ready for review. Unless support in the disassembler is required, though I'm not sure how useful it would be since dxa and the unit testing tool both provide this functionality through D3DReflectionDumper. |
Relates to microsoft/DirectX-Headers#135.
I have tested this version in my own project and it seems to work. Only thing is that this requires the DirectX-Headers dep to be updated to ensure D3D12_FUNCTION_DESC1 and co can be found.