From: "Zebediah Figura (she/her)" Subject: Re: [PATCH vkd3d 3/4] vkd3d-shader: Add support for retrieving the shader version through vkd3d_shader_scan(). Message-Id: <0b14918a-cf1c-bf63-165a-17fd889fb7e9@codeweavers.com> Date: Mon, 10 Jan 2022 19:45:45 -0600 In-Reply-To: References: <20211228222020.13814-1-zfigura@codeweavers.com> <20211228222020.13814-3-zfigura@codeweavers.com> On 1/6/22 04:15, Henri Verbeet wrote: > On Tue, 28 Dec 2021 at 23:20, Zebediah Figura wrote: >> Besides being the first patch implementing reflection, this patch introduces an >> API problem with regard to D3DReflect() that will affect many different fields. >> Namely, if the SM4 version token contains a nonsensical version D3DReflect() >> will happily return it verbatim. As I see it our options are: >> >> (1) return the token verbatim anyway instead of using struct >> vkd3d_shader_version, >> >> (2) don't use vkd3d_shader_scan() for wine's D3DReflect() [and either diverge >> from native in vkd3d-utils' D3DReflect(), or don't use vkd3d_shader_scan() >> there either] >> >> (3) return the token verbatim in a separate field (and in a separate chained >> structure?) >> >> (4) punt, and use one of the above solutions if we ever do come across a shader >> that contains insane data. >> > I'd be inclined to go with option 3 here. Somewhat analogous to e.g. > vkd3d_shader_spirv_target_info or vkd3d_shader_hlsl_source_info, we > could perhaps do something like the following: > > struct vkd3d_shader_tpf_scan_info > { > ... > uint32_t version_token; > const char *creator; > uint32_t flags; > ... > }; > > We could perhaps slightly generalise that to something like > "vkd3d_shader_d3d_scan_info", since the documentation claims the > ID3D12ShaderReflection should be supported for the d3dbc, tpf and dxil > targets, although in case of the dxil target that would normally be > provided by the dxcompiler DLL instead of d3dcompiler. I think that makes sense to me. > >> There are some other, similar problems: >> >> * HLSL compile flags would be especially annoying to translate to e.g. >> libvkd3d-shader compile arguments and then back again. (Frankly, it's already >> annoying that we have to do that in the HLSL compiler.) This is basically the >> same problem as the above, but it forms an argument for returning tokens >> verbatim, or at least that one. >> > Sure. > >> * Instruction counts (and I think everything else in the STAT chunk) are always >> taken from the STAT chunk; if the STAT chunk is missing then D3DReflect() will >> return zero, even though it could just count them. Should we preserve that >> behaviour in libvkd3d-shader? in libvkd3d-utils? >> > If we're going with something like > vkd3d_shader_tpf_scan_info/vkd3d_shader_d3d_scan_info, probably yes. Okay, I think that also makes sense. It then becomes an open question whether we should report more accurate information in struct vkd3d_shader_scan_function_info (or whatever the generic structure is) but that can be deferred until later.