[PATCH v5 vkd3d 12/13] vkd3d-shader/hlsl: Implement texture gather methods.

[Matteo Bruni]

On Wed, Jan 26, 2022 at 10:33 PM Zebediah Figura (she/her)
<zfigura at codeweavers.com> wrote:
>
> On 1/26/22 08:52, Matteo Bruni wrote:
> > On Wed, Jan 19, 2022 at 1:06 PM Matteo Bruni <matteo.mystral at gmail.com> wrote:
> >>
> >> On Wed, Dec 22, 2021 at 12:23 AM Zebediah Figura
> >> <zfigura at codeweavers.com> wrote:
> >>>
> >>> From: Francisco Casas <fcasas at codeweavers.com>
> >>>
> >>> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> >>> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
> >>> ---
> >>> v5: Strip newlines from hlsl_fixme(), get rid of the as-yet-unused
> >>> status_out_arg variable, expand the srcs[] array to have size 4, use sm4
> >>> register helpers, minor stylistic tweaks.
> >>>
> >>>   libs/vkd3d-shader/hlsl.c     |  4 ++
> >>>   libs/vkd3d-shader/hlsl.h     |  4 ++
> >>>   libs/vkd3d-shader/hlsl.y     | 99 ++++++++++++++++++++++++++++++++++++
> >>>   libs/vkd3d-shader/hlsl_sm4.c | 57 ++++++++++++++++++++-
> >>>   4 files changed, 163 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> >>> index 26175008d..856fb0f9d 100644
> >>> --- a/libs/vkd3d-shader/hlsl.c
> >>> +++ b/libs/vkd3d-shader/hlsl.c
> >>> @@ -1280,6 +1280,10 @@ static void dump_ir_resource_load(struct vkd3d_string_buffer *buffer, const stru
> >>>       {
> >>>           [HLSL_RESOURCE_LOAD] = "load_resource",
> >>>           [HLSL_RESOURCE_SAMPLE] = "sample",
> >>> +        [HLSL_RESOURCE_GATHER_RED] = "gather_red",
> >>> +        [HLSL_RESOURCE_GATHER_GREEN] = "gather_green",
> >>> +        [HLSL_RESOURCE_GATHER_BLUE] = "gather_blue",
> >>> +        [HLSL_RESOURCE_GATHER_ALPHA] = "gather_alpha",
> >>>       };
> >>>
> >>>       vkd3d_string_buffer_printf(buffer, "%s(resource = ", type_names[load->load_type]);
> >>> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> >>> index 2396adb40..49fa8d9d3 100644
> >>> --- a/libs/vkd3d-shader/hlsl.h
> >>> +++ b/libs/vkd3d-shader/hlsl.h
> >>> @@ -378,6 +378,10 @@ enum hlsl_resource_load_type
> >>>   {
> >>>       HLSL_RESOURCE_LOAD,
> >>>       HLSL_RESOURCE_SAMPLE,
> >>> +    HLSL_RESOURCE_GATHER_RED,
> >>> +    HLSL_RESOURCE_GATHER_GREEN,
> >>> +    HLSL_RESOURCE_GATHER_BLUE,
> >>> +    HLSL_RESOURCE_GATHER_ALPHA,
> >>>   };
> >>>
> >>>   struct hlsl_ir_resource_load
> >>> diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> >>> index 9b1e5c071..460ba44bb 100644
> >>> --- a/libs/vkd3d-shader/hlsl.y
> >>> +++ b/libs/vkd3d-shader/hlsl.y
> >>> @@ -1930,6 +1930,105 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
> >>>           list_add_tail(instrs, &load->node.entry);
> >>>           return true;
> >>>       }
> >>> +    else if (!strcmp(name, "Gather") || !strcmp(name, "GatherRed") || !strcmp(name, "GatherBlue")
> >>> +            || !strcmp(name, "GatherGreen") || !strcmp(name, "GatherAlpha"))
> >>> +    {
> >>> +        const unsigned int sampler_dim = sampler_dim_count(object_type->sampler_dim);
> >>> +        enum hlsl_resource_load_type load_type;
> >>> +        const struct hlsl_type *sampler_type;
> >>> +        struct hlsl_ir_resource_load *load;
> >>> +        struct hlsl_ir_node *offset = NULL;
> >>> +        struct hlsl_ir_load *sampler_load;
> >>> +        struct hlsl_type *result_type;
> >>> +        struct hlsl_ir_node *coords;
> >>> +        unsigned int read_channel;
> >>> +
> >>> +        if (!strcmp(name, "GatherGreen"))
> >>> +        {
> >
> > At some point we probably want to introduce an enum for intrinsics and
> > e.g. table-driven intrinsic_from_{function,method}() helpers to find
> > out which intrinsic a call is supposed to match, if any. Repeatedly
> > string-matching the function name like this is a bit ugly and it's
> > only going to get worse as we introduce more intrinsics.
> > The intrinsics currently in hlsl.y, intrinsic_functions[] sidestep the
> > problem but that's only going to apply to those that can be inlined
> > straight away.
>
> I agree that a table would help here. I'm not sure I understand the
> concern about intrinsics that can't be inlined, though; can you please
> elaborate?

I'm referring to those intrinsics that shouldn't become expressions
but stay as function calls, to make sure they aren't reordered or
otherwise optimized (e.g. barriers). As we discussed earlier, they
could also be represented by a separate instruction type, in which
case they are sort of inlined anyway and, I guess, the existing
intrinsic_functions[] table could be a good place for the
string-to-enum conversion I'm proposing.