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

[Zebediah Figura (she/her)]

On 12/20/21 10:14, Francisco Casas wrote:
> 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.

I don't personally mind large functions, but on the other hand, the code 
style we use in wined3d tends to be relatively liberal with helpers (as 
long as they're well-defined, I guess.)

It's a minor thing, really, just something that struck me as a possible 
improvement.

> 
>>> + 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.

It wouldn't be the first field that's only relevant to some load ops, at 
least.

> 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.

Well, it'd presumably be a HLSL_SWIZZLE_* value, so you don't need a switch.

But yes, in terms of duplicated code it's relatively anodyne I think. 
Hence more of a "have you considered" kind of question.