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

Added implementation for ID3D12FunctionReflection1::GetDesc1 #6827

Open
wants to merge 65 commits into
base: main
Choose a base branch
from

Conversation

Nielsbishere
Copy link
Contributor

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.

@Nielsbishere Nielsbishere requested a review from a team as a code owner July 25, 2024 22:36
Copy link
Contributor

github-actions bot commented Jul 25, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@Nielsbishere Nielsbishere changed the title Added implementation for ID3D12FunctionReflection::GetDesc1 Added implementation for ID3D12FunctionReflection1::GetDesc1 Jul 27, 2024
@damyanp
Copy link
Member

damyanp commented Jul 31, 2024

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!

  • Damyan.

@Nielsbishere
Copy link
Contributor Author

Sounds reasonable, though I'm not sure if you're referring to unit tests or that the tests are failing on this PR.
Mainly the payloadSize/intersectionSize and some workgraph information is going to be useful for me. In my current parsers (also in production at work) I need to do custom work to parse struct sizes and other information that could hopefully be exposed even with a clang supported version of DXC. My goal there is to move as much as possible to a standardized DXC to avoid manual parser errors and missing functionality from my side. It is working in my PR, but it's tricky since this is dependent on the DirectX-Headers change. I updated the dependency, but still need to upstream some of the changes I had to make it build properly. If this PR is ever merged, it needs to change the external dependency's URL back from Oxsomi/DirectX-Headers to Microsoft/DirectX-Headers, since I have no other way to make the tests pass until that is merged.

@damyanp
Copy link
Member

damyanp commented Jul 31, 2024

...I'm not sure if you're referring to unit tests...

I'm referring to unit tests with the code. From CONTRIBUTING.md:

All changes that make functional or behavioral changes to the compiler whether by fixing bugs or adding features must include additional testing in the implementing pull request.

…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 :)
@Nielsbishere
Copy link
Contributor Author

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.

…e. Also don't like an include named winadapter.h in a file named WinAdapter.h
…rt. Added CLSID_D3D12SDKConfiguration for older Windows sdks
…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
@Nielsbishere Nielsbishere changed the title Draft: Added implementation for ID3D12FunctionReflection1::GetDesc1 Added implementation for ID3D12FunctionReflection1::GetDesc1 Aug 24, 2024
@Nielsbishere
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

2 participants