From: Henri Verbeet <hverbeet@gmail.com>
Subject: Re: [PATCH vkd3d 3/9] vkd3d-shader: Add 64-bit immediate constant register type.
Message-Id: <CAOsNvwxxciFe7sBzHnyh3EAF-PEQ0bR3UPEru6RddRygbs4qrA@mail.gmail.com>
Date: Tue, 15 Jun 2021 23:53:03 +0200
In-Reply-To: <20210614032643.14994-3-cmccarthy@codeweavers.com>
References: <20210614032643.14994-1-cmccarthy@codeweavers.com> <20210614032643.14994-3-cmccarthy@codeweavers.com>

On Mon, 14 Jun 2021 at 05:27, Conor McCarthy <cmccarthy@codeweavers.com> 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.