Re: [PATCH vkd3d 8/8] vkd3d-shader/glsl: Implement shader_glsl_get_register_name().

[Henri Verbeet]

On Wed, 18 Aug 2021 at 06:21, Atharva Nimbalkar  wrote:
> @@ -219,13 +219,59 @@ static void shader_glsl_get_register_name(struct vkd3d_glsl_generator *gen,
>          const struct vkd3d_shader_register *reg, enum vkd3d_data_type data_type,
>          struct vkd3d_string_buffer *register_name, bool *is_swizzled)
>  {
> - /* TODO: Add implementation
> -
> * Sets an error state as of now
> -
> */
> - if(is_swizzled)
> - /*
> -
> - Currently adds support for input and output shaders.
> -
> - with support for relative addressing.
> - */
> -

"registers", not "shaders".
> - const struct vkd3d_shader_version *version = &gen->version;
> - const char *prefix = shader_get_type_prefix(version->type);
> - struct glsl_src_param rel_param0, rel_param1;
> -
> - rel_param0.param_str = vkd3d_string_buffer_get(&gen->string_buffers);
> - rel_param1.param_str = vkd3d_string_buffer_get(&gen->string_buffers);
> - if (reg->idx[0].offset != ~0u && reg->idx[0].rel_addr)
> -
>    glsl_src_param_init_ext(gen, reg->idx[0].rel_addr, VKD3DSP_WRITEMASK_0,
> -
>            &rel_param0, reg->idx[0].rel_addr->reg.data_type);
> - if (reg->idx[1].offset != ~0u && reg->idx[1].rel_addr)
> -
>    glsl_src_param_init_ext(gen, reg->idx[1].rel_addr, VKD3DSP_WRITEMASK_0,
> -
>            &rel_param1, reg->idx[1].rel_addr->reg.data_type);

"rel_param0.param_str" and "rel_param1.param_str" are also initialised
by glsl_src_param_init_ext(), so would be leaked in that case. I
assume the vkd3d_string_buffer_get() calls were added to match the
glsl_src_param_cleanup() at the end of this function. Perhaps the best
thing to do would be to only call those in the same cases that we call
glsl_src_param_init_ext() above.
> - if (is_swizzled)
>     *is_swizzled = false;
> - vkd3d_glsl_compiler_error(gen, VKD3D_SHADER_ERROR_GLSL_INTERNAL,
> -
>        "Internal compiler error: Unhandled register type %#x.", reg->type);
> -
> - switch (reg->type)
> - {
> -
>    case VKD3DSPR_INPUT:
> -
>        if (version->type == VKD3D_SHADER_TYPE_VERTEX)
> -
>        {
> -
>            if (reg->idx[0].rel_addr)
> -
>            {
> -
>                vkd3d_string_buffer_printf(register_name, "%s_in[%s + %u]",
> -
>                        prefix, rel_param0.param_str->buffer, reg->idx[0].offset);
> -
>            }
> -
>            else
> -
>            {
> -
>                vkd3d_string_buffer_printf(register_name, "%s_in%u", prefix, reg->idx[0].offset);
> -
>            }
> -
>            break;
> -
>        }
> -
>        break;

This doesn't handle inputs for other shader types. That's fine, but we
should generate an error when we encounter those.
> -
>    case VKD3DSPR_OUTPUT:
> -
>        if (reg->idx[0].rel_addr)
> -
>            vkd3d_string_buffer_printf(register_name, "%s_out[%s + %u]",
> -
>                    prefix, rel_param0.param_str->buffer, reg->idx[0].offset);
> -
>        else
> -
>            vkd3d_string_buffer_printf(register_name, "%s_out[%u]", prefix, reg->idx[0].offset);
> -
>        break;

We could conceivably split this patch in two, one implementing
handling of VKD3DSPR_OUTPUT, and the other implementing handling of
VKD3DSPR_INPUT.