2009/8/10 Stefan Dösinger <stefandoesin...@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
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], ®, sizeof(reg)); }