Re: [PATCH vkd3d 3/9] vkd3d-shader: Add 64-bit immediate constant register type.

[Henri Verbeet]

On Mon, 14 Jun 2021 at 05:27, Conor McCarthy  wrote:
> +static unsigned int vkd3d_spirv_get_component_value_count(enum vkd3d_shader_component_type component_type)
> +{
> - switch (component_type)
> - {
> -
>    case VKD3D_SHADER_COMPONENT_UINT:
> -
>    case VKD3D_SHADER_COMPONENT_INT:
> -
>    case VKD3D_SHADER_COMPONENT_FLOAT:
> -
>        return 1;
> -
>    case VKD3D_SHADER_COMPONENT_DOUBLE:
> -
>        return 2;
> -
>    default:
> -
>        ERR("Invalid shader component type %u.\n", component_type);
> -
>        return 1;
> - }
>
> +}
> -

Given that this is only used by vkd3d_dxbc_compiler_get_constant(), it
seems tempting to just integrate this with the existing switch there.
> static uint32_t vkd3d_dxbc_compiler_get_constant(struct vkd3d_dxbc_compiler *compiler,
>          enum vkd3d_shader_component_type component_type, unsigned int component_count, const uint32_t *values)
>  {
>      uint32_t type_id, scalar_type_id, component_ids[VKD3D_VEC4_SIZE];
>      struct vkd3d_spirv_builder *builder = &compiler->spirv_builder;
> - unsigned int i;
> -
>
> unsigned int i, value_count;
>
> assert(0 < component_count && component_count <= VKD3D_VEC4_SIZE);
> type_id = vkd3d_spirv_get_type_id(builder, component_type, component_count);
>
> switch (component_type)
> {
> -
>    case VKD3D_SHADER_COMPONENT_DOUBLE:
>    case VKD3D_SHADER_COMPONENT_UINT:
>    case VKD3D_SHADER_COMPONENT_INT:
>    case VKD3D_SHADER_COMPONENT_FLOAT:
>
> @@ -2629,15 +2645,17 @@ static uint32_t vkd3d_dxbc_compiler_get_constant(struct vkd3d_dxbc_compiler *com
>              return vkd3d_spirv_build_op_undef(builder, &builder->global_stream, type_id);
>      }
> - value_count = vkd3d_spirv_get_component_value_count(component_type);
> - if (component_count == 1)
>    {
> -
>    return vkd3d_spirv_get_op_constant(builder, type_id, *values);
> -
>    return vkd3d_spirv_get_op_constant(builder, type_id, values, value_count);
> }
>    else
>    {
>        scalar_type_id = vkd3d_spirv_get_type_id(builder, component_type, 1);
>        for (i = 0; i < component_count; ++i)
> -
>        component_ids[i] = vkd3d_spirv_get_op_constant(builder, scalar_type_id, values[i]);
> -
>        component_ids[i] = vkd3d_spirv_get_op_constant(builder, scalar_type_id, &values[i * value_count], value_count);
>    return vkd3d_spirv_get_op_constant_composite(builder, type_id, component_ids, component_count);
> }
>
> }

[...]
> +static uint32_t vkd3d_dxbc_compiler_emit_load_constant64(struct vkd3d_dxbc_compiler *compiler,
> -
>    const struct vkd3d_shader_register *reg, DWORD swizzle, DWORD write_mask)
>
> +{
> - unsigned int component_count = vkd3d_write_mask_component_count_typed(write_mask, VKD3D_DATA_DOUBLE);
> - uint64_t values[2] = {0};
> - unsigned int i, j;
> -
> - assert(reg->type == VKD3DSPR_IMMCONST64);
> -
> - if (reg->immconst_type == VKD3D_IMMCONST_SCALAR)
> - {
> -
>    for (i = 0; i < component_count; ++i)
> -
>        values[i] = reg->u.immconst_uint64[0];
> - }
> - else
> - {
> -
>    for (i = 0, j = 0; i < VKD3D_DVEC2_SIZE; ++i)
> -
>    {
> -
>        if (write_mask & (VKD3DSP_WRITEMASK_0 << (i * 2)))
> -
>            values[j++] = reg->u.immconst_uint64[vkd3d_swizzle_get_component(swizzle, i * 2) / 2u];
> -
>    }
> - }
> -
> - return vkd3d_dxbc_compiler_get_constant(compiler,
> -
>        vkd3d_component_type_from_data_type(reg->data_type), component_count, (const uint32_t*)values);
>
> +}
> -

That's pretty ugly though; it would seem preferable to introduce a
vkd3d_dxbc_compiler_get_constant64() that takes uint64_t values, and
build on top of that.

Note that the changes in trace.c and dxbc.c don't depend on the
changes in spirv.c, and this patch could be split.
> @@ -1038,6 +1042,42 @@ static void shader_dump_register(struct vkd3d_d3d_asm_compiler *compiler, const
>          }
>          shader_addline(buffer, ")");
>      }
> - else if (reg->type == VKD3DSPR_IMMCONST64)
> - {
> -
>    shader_addline(buffer, "(");
> -
>    switch (reg->immconst_type)
> -
>    {
> -
>        case VKD3D_IMMCONST_SCALAR:
> -
>            switch (reg->data_type)
> -
>            {
> -
>                case VKD3D_DATA_DOUBLE:
> -
>                    shader_addline(buffer, "%f", reg->u.immconst_double[0]);
> -
>                    break;
> -
>                default:
> -
>                    shader_addline(buffer, "<unhandled data type %#x>", reg->data_type);
> -
>                    break;
> -
>            }
> -
>            break;
> -
> -
>        case VKD3D_IMMCONST_VEC4:
> -
>            switch (reg->data_type)
> -
>            {
> -
>                case VKD3D_DATA_DOUBLE:
> -
>                    shader_addline(buffer, "%f, %f",
> -
>                            reg->u.immconst_double[0], reg->u.immconst_double[1]);
> -
>                    break;
> -
>                default:
> -
>                    shader_addline(buffer, "<unhandled data type %#x>", reg->data_type);
> -
>                    break;
> -
>            }
> -
>            break;
> -

This should use a helper along the lines of
shader_print_float_literal(). In particular, literals should be
printed with "compiler->colours.literal", and %f isn't necessarily
sufficient to accurately represent doubles.

You'll need a "compiler->colours.reset" before the "(" above.