Exposing some hidden CFunctionReflection members to the front end (ID3D12LibraryReflection/ID3D12FunctionReflection)

[Nielsbishere]

The DXC backend has the following parameter:
const DxilFunctionProps *m_pProps
Which has a large number of members that are not accessible by ID3D12FunctionReflection and D3D12_FUNCTION_DESC.
The most interesting ones being raytracing and workgraph related.
Exposing these saves me a lot of headache, as I won't need to parse these manually, especially since they're already available in DXC.

The following parameters still need to be exposed (and I'll take a look later to add them):

• Serialized root signature could have a getter which returns a DxcBuffer.
• InputNodes / OutputNodes could have a getter which returns a CNodeIOProperties* and length. Where each CNodeIOProperties has const char* for name as well as UINT id.
• NodeID NodeShaderId, NodeShaderSharedInput could use UINT ids in the D3D12_NODE_SHADER_DESC struct, while leaving the strings as getters.
• D3D12_SHADER_DESC might need an update to provide similar functionality.

The following parameters I won't add, since I have no idea how to properly expose them and if they're even useful to the front end:

• Vertex reflection: clipPlanes[6] returns an llvm::Constant
• Hull reflection: returns patchConstFunction as llvm::Function, which won't be accessible.

Since these are llvm internal values, I'm guessing that they might not be very useful for the front end.

This relates to a DXCompiler issue & PR.

[jenatali]

This relates to a DXCompiler issue & PR.

Please link the actual PR?

[jenatali]

I don't think we can merge a change to the D3D12_SHADER_DESC structure, since that structure is returned from both DXC and FXC, and FXC has versioning constraints that can't break. I think we can add new structures to this header, assuming the DXC changes are reviewed and accepted.

[Nielsbishere]

Okay sounds good, I'll add an extended version that can only be queried for DXC. The DXC version is on my fork, but I gotta rebase it first, since it's built on other changes.

[Nielsbishere]

Implemented by microsoft/DirectXShaderCompiler#6827 on the DXC side. Not sure if there's anything that has to be done for FXC? Maybe it shouldn't be declared "PURE" and return failed instead?

[jenatali]

Implemented by microsoft/DirectXShaderCompiler#6827 on the DXC side. Not sure if there's anything that has to be done for FXC? Maybe it shouldn't be declared "PURE" and return failed instead?

The way to do this without breaking FXC is to put it on an ID3D12FunctionReflection1 with a new IID, and then you can implement QueryInterface for it.

Specifically, the layout of the virtual function table cannot change. Inserting a function in the middle of the table means that when you call that function, or later, on a concrete implementation coming from FXC or old DXC, you're calling a different function than you think you are. You could add it to the end, but then if you try to call it on an old concrete implementation, you'll just crash because there's nothing there in the function table. Even if you have a default implementation instead of PURE, you'd need to recompile the caller, for it to have a function at that spot in the vtable.

The only way to have a clean runtime failure is to add a separate interface and query.

[Nielsbishere]

It seems like I have to expose this with ID3D12LibraryReflection1::GetFunctionByIndex1, as ID3D12FunctionReflection doesn't inherrit IUnknown, which I don't think I can safely change. I can make the new ID3D12FunctionReflection1 inherit IUnknown for the future though... But not ideal any way.

[damyanp]

There's quite a bit of API surface here that needs more careful review. So far, I've only looked at things that could be done to make the review process easier.

[Nielsbishere]

Implemented PR changes. Also exposed all remaining properties such as node info and serialized root signature.
Only thing left to do could be implementing this in D3D12_SHADER_DESC1 as well. But that could be a PR for later.

[jenatali]

I'll let the DXC folks drive whether the corresponding implementation PR in the compiler repo is appropriate, but looking solely at the API surface exposed here, this seems acceptable to me.

[damyanp]

See microsoft/DirectXShaderCompiler#6827 (comment)

We need to make sure we've completed a thorough review of all of this new API surface that'll need to be supported for a long time before accepting the change to the headers.

Thanks!

[jenatali]

Yes, agreed, I have no intentions of actually merging this until the corresponding DXC change is reviewed and completed. Assuming that the implementation is accepted, we can take this into the headers and the Windows SDK afterwards.

[jenatali]

This kind of breaking change made sense with the FXC model, where you linked against and shipped a redist D3DCompiler_xx.dll, and also makes sense with a strictly-DXC model. But in a world where D3DCompiler_47.dll is inbox in Windows and implements the old interface, where DXC can implement a new one, I think I'd prefer to rev this in a non-breaking way.

We can do that by just adding ID3D12ShaderReflection1, which inherits from ID3D12ShaderReflection and adds the new method. The VTable of ID3D12ShaderReflection1 is then identical to this new definition, except that apps can still query for the older one to avoid it being a source-breaking change for FXC consumers.

[Nielsbishere]

Okay sounds good, I was planning on doing it that way, but assumed the comment was the way to do it. Guess I'lll implement it the right way.

[Nielsbishere]

Since last time, added numthreads to mesh/amplification shaders and implemented ID3D12ShaderReflection1.
There were some... issues with upgrading DXC to the latest DirectX-Headers, since it was behind by 2 years.

Important notes: DXC uses the assumption that bool* == LPBOOL, which isn't possible if BOOL is uint32_t, this needs some further attention to see if this is OK or has side effects, but DXC and DirectX12-Headers definitely don't agree on the size of a bool... Checking the define for LLVM_SUPPORT_WIN_ADAPTER_H is used to avoid duplicate defines and structs/typedefs, this is required on Linux since basestd.h from d3dcommon.h has its own definitions that are definitely different than from DXC (dxc/WinAdapter.h).

EDIT: Going to see if I can find a way to keep BOOL as uint32_t, since it might already be serialized like that?
EDIT2: I don't think so....

[Nielsbishere]

This branch is based of #137 to allow DXC to update to latest DirectX-Headers. DirectX-Headers itself should be fine now, just need to add support for the reflection data to dxcdisassembler and update unit tests to verify dxa output.