Re: [PATCH vkd3d v3 03/12] vkd3d-shader/hlsl: Add aoffimmi modifiers on Sample sm4 instructions.

[Zebediah Figura (she/her)]

On 12/17/21 13:12, Francisco Casas wrote:
> Signed-off-by: Francisco Casas
>
> libs/vkd3d-shader/hlsl_sm4.c | 39 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c
> index 07bbd376..11e1f59a 100644
> --- a/libs/vkd3d-shader/hlsl_sm4.c
> +++ b/libs/vkd3d-shader/hlsl_sm4.c
> @@ -1306,14 +1306,47 @@ static void write_sm4_ld(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer *buf
>
> static void write_sm4_sample(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer *buffer,
>           const struct hlsl_type *resource_type, const struct hlsl_ir_node *dst,
> -
>    const struct hlsl_deref *resource, const struct hlsl_deref *sampler, const struct hlsl_ir_node *coords)
> -
>    const struct hlsl_deref *resource, const struct hlsl_deref *sampler,
> -
>    const struct hlsl_ir_node *coords, const struct hlsl_ir_node *texel_offset)
>
> {
> -
>
> struct hlsl_ir_constant *offset;
>  struct sm4_instruction instr;
>  unsigned int writemask;
>
> memset(&instr, 0, sizeof(instr));
>  instr.opcode = VKD3D_SM4_OP_SAMPLE;
> -
>
> if (texel_offset)
> -
>
> {
> -
>    struct sm4_instruction_modifier modif;
> -
>    int valid_aoffimmi = 1;
> -
> -
>    if (texel_offset->type != HLSL_IR_CONSTANT)
> -
>        valid_aoffimmi = 0;
> -
>    else
> -
>    {
> -
>        offset = hlsl_ir_constant(texel_offset);
> -
>        modif.type = VKD3D_SM4_MODIFIER_AOFFIMMI;
> -
>        modif.aoffimmi.u = offset->value[0].i;
> -
>        modif.aoffimmi.v = offset->value[1].i;
> -
>        modif.aoffimmi.w = offset->value[2].i;
> -
>        instr.modifiers[instr.modifier_count] = modif;
> -
>        instr.modifier_count++;
> -
> -
>        valid_aoffimmi &= -8 <= modif.aoffimmi.u && modif.aoffimmi.u <= 7;
> -
>        valid_aoffimmi &= -8 <= modif.aoffimmi.v && modif.aoffimmi.v <= 7;
> -
>        valid_aoffimmi &= -8 <= modif.aoffimmi.w && modif.aoffimmi.w <= 7;
> -
>    }
> -
> -
>    if (!valid_aoffimmi)

I'm not a huge fan of the way this is done; I'd prefer to see a helper, 
something like

bool encode_texel_offset(struct sm4_instruction_modifier *modifier,
         struct hlsl_ir_node *instr);

I'm also inclined to use the term "texel_offset" in general rather than 
"aoffimmi"; I have an easier time understanding the meaning of the former.
> -
>    {
> -
>        hlsl_error(ctx, &texel_offset->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
> -
>            "Offset must resolve to integer literal in the range -8 to 7\n");

INVALID_TYPE isn't really appropriate here; we probably need a new 
enumeration value.
> -
>        return;
> -
>    }
> -
> - }
> -
> sm4_register_from_node(&instr.dsts[0].reg, &instr.dsts[0].writemask, NULL, dst);
> instr.dst_count = 1;
>
> @@ -1626,6 +1659,7 @@ static void write_sm4_resource_load(struct hlsl_ctx *ctx,
>   {
>       const struct hlsl_type *resource_type = load->resource.var->data_type;
>       const struct hlsl_ir_node *coords = load->coords.node;
> -
>
> const struct hlsl_ir_node *texel_offset = load->texel_offset.node;
>
> if (load->sampler.var)
>  {
>
> @@ -1657,7 +1691,8 @@ static void write_sm4_resource_load(struct hlsl_ctx *ctx,
>           case HLSL_RESOURCE_SAMPLE:
>               if (!load->sampler.var)
>                   hlsl_fixme(ctx, &load->node.loc, "SM4 combined sample expression.");
> -
>        write_sm4_sample(ctx, buffer, resource_type, &load->node, &load->resource, &load->sampler, coords);
> -
>        write_sm4_sample(ctx, buffer, resource_type, &load->node, &load->resource, &load->sampler,
> -
>                coords, texel_offset);
>         break;
> }
> }