Re: [PATCH vkd3d 4/5] vkd3d-shader/hlsl: Parse the max() intrinsic.

[Matteo Bruni]

On Fri, Sep 3, 2021 at 6:37 PM Zebediah Figura (she/her)
 wrote:
> On 9/3/21 7:13 AM, Matteo Bruni wrote:
> > On Thu, Sep 2, 2021 at 12:23 AM Zebediah Figura  wrote:
> > > Signed-off-by: Zebediah Figura
> > >
> > > Makefile.am                 |  2 ++
> > >   libs/vkd3d-shader/hlsl.y    | 67 +++++++++++++++++++++++++++++++++++++
> > >   tests/max.shader_test       | 23 +++++++++++++
> > >   tests/shader_runner_d3d12.c |  2 ++
> > >   4 files changed, 94 insertions(+)
> > >   create mode 100644 tests/max.shader_test
> > > diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
> > > index 2512bcc0..d374f64b 100644
> > > --- a/libs/vkd3d-shader/hlsl.y
> > > +++ b/libs/vkd3d-shader/hlsl.y
> > > @@ -1555,10 +1555,38 @@ static const struct hlsl_ir_function_decl *find_function_call(struct hlsl_ctx *c
> > >       return args.decl;
> > >   }
> > >
> > > +static bool intrinsic_max(struct hlsl_ctx *ctx,
> > > -
> > >    const struct parse_initializer *params, struct vkd3d_shader_location loc)
> > >
> > > +{
> > > - struct hlsl_ir_node *args[3] = {params->args[0], params->args[1]};
> > > -
> > > - return !!add_expr(ctx, params->instrs, HLSL_OP2_MAX, args, &loc);
> > >
> > > +}
> > > -
> > >
> > > +static const struct intrinsic_function
> > > +{
> > > - const char *name;
> > > - int param_count;
> > > - bool check_numeric;
> > > - bool (*handler)(struct hlsl_ctx *ctx, const struct parse_initializer *params, struct vkd3d_shader_location loc);
> > >
> > > +}
> > > +intrinsic_functions[] =
> > > +{
> > > - {"max",                                 2, true,  intrinsic_max},
> > >
> > > +};
> > > -
> > >
> > > +static int intrinsic_function_name_compare(const void *a, const void *b)
> > > +{
> > > - const struct intrinsic_function *func = b;
> > > -
> > > - return strcmp(a, func->name);
> > >
> > > +}
> > > -
> > >
> > > static struct list *add_call(struct hlsl_ctx *ctx, const char *name,
> > >         struct parse_initializer *params, struct vkd3d_shader_location loc)
> > > {
> > >     const struct hlsl_ir_function_decl *decl;
> > > -
> > >
> > > struct intrinsic_function *intrinsic;
> > >
> > > if ((decl = find_function_call(ctx, name, params)))
> > >  {
> > >
> > > @@ -1566,6 +1594,45 @@ static struct list *add_call(struct hlsl_ctx *ctx, const char *name,
> > >           free_parse_initializer(params);
> > >           return NULL;
> > >       }
> > > - else if ((intrinsic = bsearch(name, intrinsic_functions, ARRAY_SIZE(intrinsic_functions),
> > > -
> > >        sizeof(*intrinsic_functions), intrinsic_function_name_compare)))
> > > - {
> > > -
> > >    if (intrinsic->param_count >= 0 && params->args_count != intrinsic->param_count)
> > > -
> > >    {
> > > -
> > >        hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
> > > -
> > >                "Wrong number of arguments to function '%s': expected %u, but got %u.\n",
> > > -
> > >                name, intrinsic->param_count, params->args_count);
> > > -
> > >        free_parse_initializer(params);
> > > -
> > >        return NULL;
> > > -
> > >    }
> >
> > I guess the idea is to use param_count == 0 to mark intrinsic
> > functions with a variable number of parameters? I think it would be
> > better to use a different number (e.g. -1 if you keep int for
> > param_count, or ~0u otherwise) given that we probably want to check
> > that calls to e.g. AllMemoryBarrier() don't have arguments.
>
> The idea is to use -1. That's a >=, not a >.

Indeed, I can't read.