D3D shader assembler (round 2)

[Matteo Bruni]

2009/8/10 Stefan Dösinger <stefandoesinger at gmx.at>:
> Hi,
> A few comments - mostly things I haven't spotted earlier.

Hi,
I fixed the code, almost completely following your reviews (thank you,
of course).
I'm reporting the differences with your suggestions:

>
>> --- /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.
>

I merged everything into d3dx9_36_private.h

>> +%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.
>

Removing the underscore from the flex prefix produces a (better)
asmshader_scan_string function but also an ugly asmshaderlex_init. I
decided to keep the underscore.

>> +    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?
>

I named them BWRITERxxxx (the same for wine_shader, now it is named
bwriter_shader, enforcing the idea that that represents the bytecode
writer interface). Is it ok?

>>+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?
>

It's not needed, but I kept it in anticipation of D3D10 support. Can
be removed anyway.

> 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)

Looking better at this, I noticed that I wasn't checking the register
number, while native has this check. So I reworked this part, and now
the code is as the attached file.

Regarding Henri's review, I followed everything, except keeping
my_alloc/realloc/free (now asm_alloc and c.) functions, as it seems
Stefan likes them very much :)

Supposing my changes are ok, what should I do now? Wait for reviews of
the rest? Or maybe send the updated code? In this case, should I send
it again in wine-devel or have a try with wine-patches?

Cheers,
Matteo Bruni
-------------- next part --------------
static BOOL check_reg_type(const struct shader_reg *reg,
                           const struct allowed_reg_type *allowed) {
    unsigned int i = 0;

    while(allowed[i].type != ~0U) {
        if(reg->type == allowed[i].type) {
            if(reg->rel_reg) return TRUE; /* The relative addressing register
                                             can have a negative value, we
                                             can't check the register index */
            if(reg->regnum < allowed[i].count) return TRUE;
            else return FALSE;
        }
        i++;
    }
    return FALSE;
}

/* Native compiler doesn't make separate checks for src and dst registers */
struct allowed_reg_type vs_1_reg_allowed[] = {
    { BWRITERSPR_TEMP,     12 },
    { BWRITERSPR_INPUT,    16 },
    { BWRITERSPR_CONST,   ~0U },
    { BWRITERSPR_ADDR,      1 },
    { BWRITERSPR_RASTOUT,   3 }, /* oPos, oFog and oPts */
    { BWRITERSPR_ATTROUT,   2 },
    { BWRITERSPR_TEXCRDOUT, 8 },
    { ~0U, 0 } /* End tag */
};

static void asmparser_srcreg_vs_1(struct asm_parser *This,
                                  struct instruction *instr, int num,
                                  const struct shader_reg *src) {
    struct shader_reg reg;

    if(!check_reg_type(src, vs_1_reg_allowed)) {
        asmparser_message(This, "Line %u: Source register %s not supported in VS 1\n",
                          This->line_no,
                          debug_print_srcreg(src, ST_VERTEX));
        set_parse_status(This, PARSE_ERR);
    }
    check_legacy_srcmod(This, src->srcmod);
    check_abs_srcmod(This, src->srcmod);
    reg = map_oldvs_register(src);
    memcpy(&instr->src[num], &reg, sizeof(reg));
}