D3D shader assembler (round 2)

[Matteo Bruni]

I'm sending a new version of the shader assembler, which tries to
address the caveats and suggestions previously given by Henri Verbeet
(http://www.winehq.org/pipermail/wine-devel/2009-July/077428.html for
reference) and has a few other changes, such as making the parser
reentrant and matching more faithfully native behavior in handling
errors in shader programs.
One point of discussion could be the internal transformations done to
shaders version 1.x and 2.x, towards a sm 3.0-type representation.
This is not really necessary for the assembler (which could naively
keep the original instruction opcode and register type), but I think
it can be useful looking at a reuse of the bytecode writer in the
shader compiler: this way the compiler could be somewhat simplified,
having to deal just with sm 3.0 registers and conventions, while it
would be writer's responsibility to convert to the destination format
(e.g. transforming some mov to texcoord in <= ps 1.3). Anyway, if this
is not considered useful, it could be removed.
As usual, reviews, comments and suggestions are much appreciated.

Regards,
Matteo Bruni.

[Stefan Dösinger]

Hi,
A few comments - mostly things I haven't spotted earlier.
> --- /dev/null
> +++ b/dlls/d3dx9_36/asmshader.h
> --- /dev/null
> +++ b/dlls/d3dx9_36/asmshader_private.h

I think it doesn't need two include files, especially since both of them are 
d3dx9-private anyway. The question is, is it better to just merge them into 
d3dx9_36_private.h, or have assembler things in a separate file? I tend 
towards putting everything into d3dx9_36_private.h, but I don't have a strong 
opinion about that.
> +%option prefix="asmshader_"
> - buffer = asmshader__scan_string(text, asm_ctx.yyscanner);

It seems that flex adds an underscore to the prefix anyway, so I guess its 
fine to remove the one from the prefix option.
> -
>   | VER_DX8_VS11
> -
>    {
> -
>            TRACE("Vertex shader 1.1 dx8\n");
> -
>               /* TODO: create the appropriate parsercontext*/
> -
>    }

Is that still needed? I know that the "vs.1.1" start token is still valid in 
d3dx9, but afair it behaves like vs_1_1, so this can be handed in the lexer 
rule
> - Here below there are some enumerations and defines previously included
> - from wined3d_private_types.h

My long term experience is that after a certain period of time comments where 
the code came from(as opposed to why it was written that way, and not some 
other way) aren't helpful any more. Probably just remove the comment, and 
also see the point below:
> - WINED3DSIO_NOP = 0,
> - WINED3DSIO_MOV = 1,
> - WINED3DSIO_ADD = 2,
>
> and others

Can you give them a different name prefix, to make it clear that they don't 
have any relationship with wined3d any more?
> +DWORD SlWriteBytecode(const wine_shader *shader, int dxversion, DWORD

**result) {
> +static struct bc_writer *create_writer(DWORD version, DWORD dxversion) {

Does it still need the dxversion?
> - unsigned int line_no;
>
> ...
> -
>    asmparser_message(This, "Line %d: Source modifier %s not supported 

in this shader version\n",
> -
>                      This->line_no,
> -
>                      debug_print_srcmod(srcmod));

Please use %u for unsigned ints(happens in quite a few places)

asmparser_srcreg_vs_3:
> - if(src->type != WINED3DSPR_TEMP && src->type != WINED3DSPR_INPUT &&
> -
>   src->type != WINED3DSPR_CONST && src->type != WINED3DSPR_ADDR &&
> -
>   src->type != WINED3DSPR_CONSTBOOL && src->type != 

WINED3DSPR_CONSTINT &&
> -
>   src->type != WINED3DSPR_LOOP && src->type !=WINED3DSPR_SAMPLER &&
> -
>   src->type != WINED3DSPR_LABEL && src->type != WINED3DSPR_PREDICATE) 

A swich-case statement or a lookup table would probably be nicer. (A lookup 
table has the disadvantage that you still have to check min and max 
separately though)

Still looking at the remaining code, so more might follow if I find more.

[Henri Verbeet]

I only looked at the first patch.

2009/8/9 Matteo Bruni :
> +static void asmparser_instr(struct asm_parser *This, DWORD opcode,

...
> - if(srcs) {
> -
>    for(i=0; i<srcs->nr; i++) {

...
> - if(srcs && (srcs->nr != expectednsrcs)) {

...
> - if(srcs) {
> -
>    for(i=0; i<srcs->nr; i++) {

You can simplify that a bit by doing something like "unsigned int
src_count = srcs ? src->nr : 0;" at the start, and then just using
src_count.
> - rc = vsnprintf(base, MESSAGEBUFFER_SIZE - 1 - ctx->messagesize, fmt, args);

You should include wine/port.h if you use vsnprintf(). It's probably
not a bad idea to just include that in general.
> +typedef struct wine_shader_ {

That's not very nice (both the name and the typedef).
> +static inline LPVOID my_alloc(SIZE_T size) {

You'll want to give that a better name than "my_alloc", although I
wonder if you need a function for this at all.
> +struct src_regs {
> - struct shader_reg reg[MAX_SRC_REGS];
> - int               nr;
>
> +};

I think "nr" can be unsigned. Personally I'd named the variable "count".
> +#define MESSAGEBUFFER_SIZE 4096
> +struct asm_parser {
> - /* The function table of the parser implementation */
> - struct asmparser_backend *funcs;
> -
> - /* Private data follows */
> - wine_shader              *shader;
> - unsigned int              m3x3pad_count;
> -
> - /* Reentrant lexer pointer */
> - void *yyscanner;
> -
> - enum parse_status{
> -
>    PARSE_SUCCESS = 0,
> -
>    PARSE_WARN = 1,
> -
>    PARSE_ERR = 2
> - } status;
> - char messages[MESSAGEBUFFER_SIZE];
> - unsigned int messagesize;
> - unsigned int line_no;
>
> +};

A dynamically allocated message buffer wouold probably be better.
> +#define SWIZZLE_ERR -1

...
> -
>            return SWIZZLE_ERR; /* 0 is a valid swizzle */

DWORDs are unsigned, so you should define SWIZZLE_ERR to something
like ~0U. Also, if you allow text2swizzle() to return an error, you
should check for it when calling the function.
> +void add_instruction(wine_shader *shader, struct instruction *instr) {
> - struct instruction      **new_instructions;
> -
> - if(shader->instr_alloc_size == 0) {
> -
>    shader->instr = my_alloc(sizeof(*shader->instr));
> -
>    shader->instr_alloc_size = 1;
> - } else if(shader->instr_alloc_size == shader->num_instrs) {
> -
>    shader->instr_alloc_size += max(shader->instr_alloc_size, 1);
> -
>    new_instructions = my_realloc(shader->instr,
> -
>                                  sizeof(*shader->instr) * (shader->instr_alloc_size));
> -
>    if(!new_instructions) {
> -
>        ERR("Failed to grow the shader instruction array\n");
> -
>        return;
> -
>    }
> -
>    shader->instr = new_instructions;
> - } else if(shader->num_instrs > shader->instr_alloc_size) {
> -
>    ERR("More instructions than allocated. This should not happen\n");
> - }
> -
> - shader->instr[shader->num_instrs] = instr;
> - shader->num_instrs++;
>
> +}

This function can fail, so the caller needs to handle the failure.
> - if(shader->num_cf) {
> -
>    shader->constF = my_realloc(shader->constF,
> -
>                                sizeof(*shader->constF) * (shader->num_cf + 1));
> - } else {
> -
>    shader->constF = my_alloc(sizeof(*shader->constF));
> - }
> -
> - newconst = my_alloc(sizeof(*newconst));

These can fail. Some of the other functions have similar flaws.
> -
>    shader->samplers = my_realloc(shader->samplers,
> -
>                                  sizeof(*shader->samplers) * (shader->num_samplers + 1));
> - }
> - if(!shader->samplers) {
> -
>    ERR("Out of memory\n");
> -
>    return FALSE;
> - }

If my_realloc() fails, you've now leaked the original shader->samplers memory.
> - /* Allocate 8 DWORDs for the start - even a minimalistic shader will need
> -
> * some instructions. Allocating it here makes the growing code easier since
> -
> * it just needs a realloc then
> -
> */
> - ret->alloc_size = 8;

I think you should aim for an "average" shader rather than a minimal one here.
> -
>    buffer->data = my_realloc(buffer->data,
> -
>                              sizeof(DWORD) * buffer->alloc_size);
> -
>    if(!buffer->data) {
> -
>        ERR("Failed to grow the buffer data memory\n");
> -
>        buffer->state = E_OUTOFMEMORY;
> -
>    }

Similar to record_sampler() above, you'll leak the original
buffer->data memory if my_realloc() fails.