[PATCH vkd3d v2 5/6] vkd3d-shader: Write the SM1 constant table.

[Matteo Bruni]

On Tue, Apr 20, 2021 at 10:15 PM Zebediah Figura (she/her)
<zfigura at codeweavers.com> wrote:
>
> On 4/20/21 3:16 AM, Matteo Bruni wrote:
> > On Fri, Apr 16, 2021 at 2:04 AM Zebediah Figura <zfigura at codeweavers.com> wrote:
> >>
> >> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
> >> ---
> >>  Makefile.am                      |   2 +
> >>  include/.gitignore               |   1 +
> >>  include/vkd3d_d3d9types.h        |   6 +
> >>  include/vkd3d_d3dx9shader.idl    |  76 +++++++++
> >>  libs/vkd3d-shader/hlsl.h         |   7 +-
> >>  libs/vkd3d-shader/hlsl.y         |   1 +
> >>  libs/vkd3d-shader/hlsl_codegen.c | 254 ++++++++++++++++++++++++++++++-
> >>  7 files changed, 344 insertions(+), 3 deletions(-)
> >>  create mode 100644 include/vkd3d_d3dx9shader.idl
> >>
> >> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> >> index 3a0bf38d..3dc7a26b 100644
> >> --- a/libs/vkd3d-shader/hlsl.h
> >> +++ b/libs/vkd3d-shader/hlsl.h
> >> @@ -115,7 +115,6 @@ struct hlsl_type
> >>      unsigned int modifiers;
> >>      unsigned int dimx;
> >>      unsigned int dimy;
> >> -    unsigned int reg_size;
> >>      union
> >>      {
> >>          struct list *elements;
> >> @@ -125,6 +124,9 @@ struct hlsl_type
> >>              unsigned int elements_count;
> >>          } array;
> >>      } e;
> >> +
> >> +    unsigned int reg_size;
> >> +    unsigned int bytecode_offset;
> >>  };
> >>
> >>  struct hlsl_struct_field
> >> @@ -134,7 +136,9 @@ struct hlsl_struct_field
> >>      struct hlsl_type *type;
> >>      const char *name;
> >>      const char *semantic;
> >> +
> >>      unsigned int reg_offset;
> >> +    unsigned int name_offset;
> >
> > Although these are indeed two offsets, they offset into very different
> > things. I don't know that I have any good suggestions
> > (name_bc_offset?) and I guess I'm not too disturbed by the current
> > naming anyway, but I feel there is room for improvement.
>
> Yes, you're right; honestly the mistake here is grouping them together
> via whitespace I think, but of course the naming is what led me to do
> that. "name_bytecode_offset" strikes me as at least a better name than
> "name_offset".
>
> I've vacillated for a while, also; it may make more sense not to store
> reg_offset + reg_size but instead to calculate them as needed. But I've
> tried that approach a couple times and the code doesn't look prettier :-(
>
> >
> >> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> >> index 25227057..73f8bdb0 100644
> >> --- a/libs/vkd3d-shader/hlsl_codegen.c
> >> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> >
> >> +    LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry)
> >> +    {
> >> +        if (!var->semantic && var->reg.allocated)
> >> +        {
> >> +            put_dword(buffer, 0); /* name */
> >> +            put_dword(buffer, D3DXRS_FLOAT4 | (var->reg.id << 16));
> >
> > This is fine for now, but maybe a comment mentioning that we might not
> > always want the float register set here would be useful.
> > Relatedly, we might have the same variable allocated to multiple
> > register sets (e.g. if it's used in a MOV and in a FOR) so in theory
> > there needs to be a loop (and we need to support multiple register
> > allocations per var, or something to the same effect). Again, no need
> > to change the code right away, just something that might be worth a
> > comment.
> >
>
> Fair enough. Actually I'd be inclined to handle this by creating
> multiple hlsl_ir_var objects.

That sounds better than my idea.