On Fri, Aug 20, 2021 at 1:45 AM Zebediah Figura zfigura@codeweavers.com wrote:
Signed-off-by: Zebediah Figura zfigura@codeweavers.com
include/private/vkd3d_common.h | 14 +++ include/vkd3d_d3dcommon.idl | 37 ++++++ libs/vkd3d-shader/hlsl.h | 6 + libs/vkd3d-shader/hlsl_codegen.c | 59 ++++++--- libs/vkd3d-shader/hlsl_sm4.c | 200 ++++++++++++++++++++++++++++++- 5 files changed, 300 insertions(+), 16 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index b5f7832f..939b356c 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -1012,13 +1012,26 @@ static void allocate_temp_registers(struct hlsl_ctx *ctx, struct hlsl_ir_functio
static void allocate_semantic_register(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, unsigned int *counter, bool output) {
static const char *shader_names[] =
{
[VKD3D_SHADER_TYPE_PIXEL] = "Pixel",[VKD3D_SHADER_TYPE_VERTEX] = "Vertex",[VKD3D_SHADER_TYPE_GEOMETRY] = "Geometry",[VKD3D_SHADER_TYPE_HULL] = "Hull",[VKD3D_SHADER_TYPE_DOMAIN] = "Domain",[VKD3D_SHADER_TYPE_COMPUTE] = "Compute",};
unsigned int type;
uint32_t reg;
bool builtin;
assert(var->semantic.name);
if (ctx->profile->major_version < 4) {
D3DSHADER_PARAM_REGISTER_TYPE type;uint32_t reg, usage_idx; D3DDECLUSAGE usage;
uint32_t usage_idx; if (!hlsl_sm1_usage_from_semantic(&var->semantic, &usage, &usage_idx)) {@@ -1027,19 +1040,35 @@ static void allocate_semantic_register(struct hlsl_ctx *ctx, struct hlsl_ir_var return; }
if (hlsl_sm1_register_from_semantic(ctx, &var->semantic, output, &type, ®)){TRACE("%s %s semantic %s[%u] matches predefined register %#x[%u].\n",ctx->profile->type == VKD3D_SHADER_TYPE_PIXEL ? "Pixel" : "Vertex", output ? "output" : "input",var->semantic.name, var->semantic.index, type, reg);}else
if ((!output && !var->last_read) || (output && !var->first_write))return;builtin = hlsl_sm1_register_from_semantic(ctx, &var->semantic, output, &type, ®);
This appears to slip in a functional change, where now we don't allocate registers for SM1 "user" (not system value / predefined register) inputs / outputs if they're unused. That's not the case as this is just moving a check from allocate_semantic_registers(). As it turns out, things aren't quite working right for those semantics anyway.
Let me go fully OT and digress into the details of this SM1 issue. Assume we have a PS with this signature:
float4 main(in float4 color : COLOR, in float4 normal : NORMAL) : SV_TARGET
and compile it for the ps_2_0 profile. If both our inputs are used, they will be both allocated to v0, which seems bad. If normal isn't used, because of the change above it won't be allocated, but since write_sm1_semantic_dcl() doesn't check for the allocated flag it will generate a dcl instruction anyway (to v0, since the allocation info is zero-initialized).
There are a few things to fix here. First of all, these "user" semantics are supposed to be mapped to the t# registers in ps_2_0 (there are 2 color inputs, v0-1, and 8 t0-7 "texcoords", but actually, generic inputs). Then we need to make sure to not step onto registers used by "builtin" semantics. I suggest reworking allocate_semantic_registers() so that both builtin and user inputs / outputs get allocation info; also keep track of the register type for allocations (maybe as SM-specific values); make sure that user inputs / outputs don't step over already allocated registers. Finally, have everything downstream from register allocation solely refer to the allocation data. I haven't really tried to write the patches so there might be something / a lot missing in the above.
diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index 38c71626..894c513f 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c
+bool hlsl_sm4_usage_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_semantic *semantic,
bool output, D3D_NAME *usage)+{
- unsigned int i;
- static const struct
- {
const char *name;bool output;enum vkd3d_shader_type shader_type;D3DDECLUSAGE usage;- }
- semantics[] =
- {
{"position", false, VKD3D_SHADER_TYPE_GEOMETRY, D3D_NAME_POSITION},{"sv_position", false, VKD3D_SHADER_TYPE_GEOMETRY, D3D_NAME_POSITION},{"sv_primitiveid", false, VKD3D_SHADER_TYPE_GEOMETRY, D3D_NAME_PRIMITIVE_ID},{"position", true, VKD3D_SHADER_TYPE_GEOMETRY, D3D_NAME_POSITION},{"sv_position", true, VKD3D_SHADER_TYPE_GEOMETRY, D3D_NAME_POSITION},{"sv_primitiveid", true, VKD3D_SHADER_TYPE_GEOMETRY, D3D_NAME_PRIMITIVE_ID},{"position", false, VKD3D_SHADER_TYPE_PIXEL, D3D_NAME_POSITION},{"sv_position", false, VKD3D_SHADER_TYPE_PIXEL, D3D_NAME_POSITION},{"color", true, VKD3D_SHADER_TYPE_PIXEL, D3D_NAME_TARGET},{"depth", true, VKD3D_SHADER_TYPE_PIXEL, D3D_NAME_DEPTH},{"sv_target", true, VKD3D_SHADER_TYPE_PIXEL, D3D_NAME_TARGET},{"sv_depth", true, VKD3D_SHADER_TYPE_PIXEL, D3D_NAME_DEPTH},{"sv_position", false, VKD3D_SHADER_TYPE_VERTEX, D3D_NAME_UNDEFINED},{"position", true, VKD3D_SHADER_TYPE_VERTEX, D3D_NAME_POSITION},{"sv_position", true, VKD3D_SHADER_TYPE_VERTEX, D3D_NAME_POSITION},- };
Eventually we might want to introduce bitmask flags for the shader type and group up identical lines together (I imagine this will become quite a bit more verbose with tessellation shaders).
- for (i = 0; i < ARRAY_SIZE(semantics); ++i)
- {
if (!ascii_strcasecmp(semantic->name, semantics[i].name)&& output == semantics[i].output&& ctx->profile->type == semantics[i].shader_type&& !ascii_strncasecmp(semantic->name, "sv_", 3)){*usage = semantics[i].usage;return true;}- }
- if (!ascii_strncasecmp(semantic->name, "sv_", 3))
return false;- *usage = D3D_NAME_UNDEFINED;
- return true;
+}
I don't quite understand what's going on here. If I read it correctly you're basically ignoring all the table entries that don't start with "sv_". Is there some planned extension of this function where it will start to do something different?
+static void write_sm4_signature(struct hlsl_ctx *ctx, struct dxbc_writer *dxbc, bool output) +{
- struct vkd3d_bytecode_buffer buffer = {0};
- struct vkd3d_string_buffer *string;
- const struct hlsl_ir_var *var;
- size_t count_position;
- unsigned int i;
- bool ret;
- count_position = put_u32(&buffer, 0);
- put_u32(&buffer, 8); /* unknown */
- LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry)
- {
unsigned int width = (1u << var->data_type->dimx) - 1, use_mask;enum vkd3d_sm4_register_type type;uint32_t usage_idx, reg_idx;D3D_NAME usage;if ((output && !var->is_output_semantic) || (!output && !var->is_input_semantic))continue;ret = hlsl_sm4_usage_from_semantic(ctx, &var->semantic, output, &usage);assert(ret);usage_idx = var->semantic.index;if (!hlsl_sm4_register_from_semantic(ctx, &var->semantic, output, &type, ®_idx)){assert(var->reg.allocated);type = VKD3D_SM4_RT_INPUT;reg_idx = var->reg.id;}
Somewhat similarly here, type doesn't seem to be used at all (yet).
use_mask = width; /* FIXME: accurately report use mask */if (output)use_mask = 0xf ^ use_mask;
Huh, that's interesting. I wonder if we're taking that into account elsewhere.