[PATCH vkd3d v3 05/12] vkd3d-shader/hlsl: Add Gather methods with offset and tests.

[Francisco Casas]

December 17, 2021 8:13 PM, "Zebediah Figura (she/her)" <zfigura at codeweavers.com> wrote:

> Just to nitpick, how about "gather_r" or "gather_red"? Just because the 4 is there in DXBC doesn't
> mean it has any semantic meaning.

Okay, I will change those.
 
> I think the assert is a bit superfluous.
> 
> On the other hand, when this kind of code pattern comes up, I tend to prefer using a helper, along
> the lines of
> 
> else if (!strcmp(name, "GatherGreen"))
> {
> add_gather(HLSL_RESOURCE_GATHER_GREEN, 1, ...);
> }
> 
> which avoids that kind of implicit agreement.

I am more inclined to just remove the assert.
I must confess that I don't like jumping from function definition to function definition too much,
so I prefer not to make new functions unless they will be called in two or more different places.

But if you insist I will add the helper, and not follow that policy of mine too much.
In that case I think it would also make sense to add helpers for Load and Sample too.
This since the code for these methods is fairly similar, so I think it would be better to keep 
them close, also for the sake of consistency.

>> + if (status_out_arg != -1)
>> + FIXME("Ignoring 'status' output parameter.\n");
> 
> This probably should be hlsl_fixme().
> 
>> +
>> + if (params->args_count == 6 || params->args_count == 7)
>> + FIXME("Ignoring multiple offset parameters.\n");
> 
> And this pretty certainly should.
> 

Okay! I see.

>> + int n_srcs = 0;
> 
> I would just leave out this variable and use "instr.src_count" directly.

Okay.

> Hmm, looking at this, it is a little tempting to add a separate field instead. Have you considered
> that approach?

I have considered adding a "channel" field to struct hlsl_ir_resource_load and only
have one value for the Gather* methods in enum hlsl_resource_load_type.

The disadvantage of the "channel" field is that it won't be used by other resource load 
instructions, and that dump_ir_resource_load() may get a little more complicated.
If you think that's ok I can change it. 
FWIW I don't think these switch cases hurt too much, 
and eventually a function or a switch would be needed to map "channel" to the right swizzle anyways.