On 12/20/21 10:14, Francisco Casas wrote:
December 17, 2021 8:13 PM, "Zebediah Figura (she/her)" <zfigura(a)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.