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

[Francisco Casas]

December 17, 2021 6:33 PM, "Zebediah Figura (she/her)"  wrote:
> 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.

I think it is better to keep using "aoffimmi" to refer to the immediate integer offset 
instruction modifiers, since the terms are not exchangeable.

For instance, for the Gather method, texel offsets not necessarily have to be represented using
aoffimmis. The 'gather4' instruction accepts aoffimmis but there is also the 'gather4_po'
instruction which receives the texel offset as an additional parameter.

I think it makes sense to add a helper to avoid duplicating code.
Since whether the texel offset is constant and inside the [-8,7] range or not may affect the 
choice of the instruction opcode, I think it would make sense to write it as:

bool encode_texel_offset_as_aoffimmi(struct hlsl_ir_node *instr, 
        const struct hlsl_ir_node *texel_offset)

so that it returns false if the texel offset is not a valid aoffimmi and the function to write 
the sm4 instruction can throw an error or fallback to another opcode accordingly.
> > - {
> > - 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;
> > }
> > }