I think this is in pretty decent shape at this point. Still a couple of comments though:
From patch 1/5:
@@ -3510,16 +3512,23 @@ static uint32_t spirv_compiler_emit_load_constant(struct spirv_compiler *compile } } - return spirv_compiler_get_constant(compiler, + val_id = spirv_compiler_get_constant(compiler, vkd3d_component_type_from_data_type(reg->data_type), component_count, values); + if (reg->immconst_data_type != reg->data_type) + val_id = vkd3d_spirv_build_op_bitcast(builder, vkd3d_spirv_get_type_id(builder, + vkd3d_component_type_from_data_type(reg->immconst_data_type), component_count), val_id); + + return val_id; }
I don't get it; spirv_compiler_get_constant() creates a constant of the requested type out of the raw bits stored in "values", why do we need a SpvOpBitcast on top of that in the SPIR-V?
From patch 2/5:
+static void shader_instruction_eliminate_phase_instance_id(struct vkd3d_shader_instruction *ins, + unsigned int instance_id) +{ + struct vkd3d_shader_register *reg; + unsigned int i; + + for (i = 0; i < ins->src_count; ++i) + { + reg = (struct vkd3d_shader_register *)&ins->src[i].reg; + if (shader_register_is_phase_instance_id(&ins->src[i].reg)) + { + reg->type = VKD3DSPR_IMMCONST; + reg->precision = VKD3D_SHADER_REGISTER_PRECISION_DEFAULT; + reg->non_uniform = false; + reg->data_type = VKD3D_DATA_UINT; + reg->idx[0].offset = ~0u; + reg->idx[0].rel_addr = NULL; + reg->idx[1].offset = ~0u; + reg->idx[1].rel_addr = NULL; + reg->idx[2].offset = ~0u; + reg->idx[2].rel_addr = NULL; + reg->immconst_type = VKD3D_IMMCONST_SCALAR; + reg->immconst_data_type = VKD3D_DATA_FLOAT; + reg->u.immconst_uint[0] = instance_id; + continue; + } + shader_register_eliminate_phase_addressing(reg, instance_id); + } + + for (i = 0; i < ins->dst_count; ++i) + shader_register_eliminate_phase_addressing((struct vkd3d_shader_register *)&ins->dst[i].reg, instance_id); +}
Oh, is this perhaps related to what patch 1/5 is intended to address? I.e., are we supposed to replace VKD3DSPR_FORKINSTID/VKD3DSPR_JOININSTID with either an integer or a float constant depending on the instruction reading it? In that case we should probably just do that by writing to reg->u.immconst_uint/reg->u.immconst_float/immconst_uint64/immconst_double depending on the data type of the original register.
+struct shader_phase_location_array +{ + struct shader_phase_location locations[MAX_REG_OUTPUT]; + unsigned int count; +};
Does MAX_REG_OUTPUT make sense as the array size above? It's not inherently tied to the number of output registers, or is it?
+void shader_normaliser_destroy(struct vkd3d_shader_normaliser *normaliser) +{ + shader_instruction_array_destroy(&normaliser->instructions); +} \ No newline at end of file
Missing newline.
@@ -6887,27 +6830,12 @@ static void spirv_compiler_emit_hull_shader_main(struct spirv_compiler *compiler if (compiler->use_vocp) spirv_compiler_emit_hull_shader_barrier(compiler); - for (i = 0; i < compiler->shader_phase_count; ++i) - { - phase = &compiler->shader_phases[i]; - if (is_control_point_phase(phase)) - continue; - - if (phase->instance_count) - { - for (j = 0; j < phase->instance_count; ++j) - { - phase_instance_id = spirv_compiler_get_constant_uint(compiler, j); - vkd3d_spirv_build_op_function_call(builder, - void_id, phase->function_id, &phase_instance_id, 1); - } - } - else - { - vkd3d_spirv_build_op_function_call(builder, void_id, phase->function_id, NULL, 0); - } - } - + /* TODO: only call the patch constant function for invocation 0. The simplest way + * is to avoid use of private variables there, otherwise we would need a separate + * patch constant epilogue also only called from invocation 0. */ + spirv_compiler_emit_hull_shader_barrier(compiler); + vkd3d_spirv_build_op_function_call(builder, void_id, spirv_compiler_get_fork_or_join_phase(compiler)->function_id, + NULL, 0);
It's different from what I commented on last time now, but this still fundamentally changes the spirv_compiler_emit_hull_shader_barrier() call from a conditional call to an unconditional call.
@@ -9965,20 +9889,32 @@ int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, const struct vkd3d_shader_compile_info *compile_info, struct vkd3d_shader_parser *parser, struct vkd3d_shader_code *spirv) { - const struct vkd3d_shader_instruction_array *instructions = &parser->instructions; const struct vkd3d_shader_spirv_target_info *info = compiler->spirv_target_info; + struct vkd3d_shader_instruction_array *instructions = &parser->instructions; const struct vkd3d_shader_spirv_domain_shader_target_info *ds_info; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + struct vkd3d_shader_normaliser normaliser; const struct vkd3d_shader_phase *phase; enum vkd3d_result result = VKD3D_OK; unsigned int i; - for (i = 0; i < instructions->count; ++i) + if (compiler->shader_type == VKD3D_SHADER_TYPE_HULL) { - if ((result = spirv_compiler_handle_instruction(compiler, &instructions->elements[i])) < 0) + shader_normaliser_init(&normaliser, instructions); + if ((result = shader_normaliser_flatten_hull_shader_phases(&normaliser)) < 0) return result; + instructions = &normaliser.instructions; }
This leaks the normaliser (specifically, the instruction array it got from the parser) on shader_normaliser_flatten_hull_shader_phases() failure.
@@ -9904,6 +9904,14 @@ int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, if ((result = shader_normaliser_flatten_hull_shader_phases(&normaliser)) < 0) return result; instructions = &normaliser.instructions; + + if (TRACE_ON()) + { + struct vkd3d_shader_parser tmp_parser; + tmp_parser.shader_version = parser->shader_version; + tmp_parser.instructions = *instructions; + vkd3d_shader_trace(&tmp_parser); + }
That looks a little fragile. I could live with it, but it looks like an argument for passing the instruction array and shader version instead of the parser to vkd3d_shader_trace() and vkd3d_dxbc_binary_to_text(). There may even be an implied argument for storing the shader version in the vkd3d_shader_instruction_array structure.