Re: [3/4] d3dx9: Implement D3DXAssembleShader function, really basic shader assembler.
2009/12/29 Henri Verbeet : > 2009/12/29 Matteo Bruni : >> I added the fake parser for two reasons: because I have to initialize >> an asm_parser structure anyway in the create__parser functions >> (and I prefer to use parser_fake for the unimplemented shader versions >> instead of parser_vs_3) and I want to avoid having some tests for the >> other shader versions pass "by chance" inside the todo_wine (this is >> accomplished by the asmparser_end_fake function... probably this can >> be seen as a hack). >> Agreed on the structure fields. >> > Can't you just let the parser fail when it encounters an unsupported > shader version? That's more or less what happens anyway, but I don't > think you have to wait until after parsing all the instructions for > that. > Yep, you are right. I believe previously I had found no way to halt the parser early, so I simply let it go until the end and I needed the fake backend to cope with that. Seems like the YYABORT macro does just what I need...
Re: [3/4] d3dx9: Implement D3DXAssembleShader function, really basic shader assembler.
On 12/29/2009 04:54 PM, Matteo Bruni wrote: 2009/12/29 Alexandre Julliard: Matteo Bruni writes: + +%option reentrant bison-bridge These won't work on old flex versions, and will get you in trouble with the flex police (aka Michael Stefaniuc ;-) All that because the Wine maintainer puts a lot of emphasis on portability ... Alexandre, now you have another person on your side against RHEL 5 and old flex versions :) I have just submitted a patch for configure.ac to require a newer flex version to build Wine. People on RHEL5 can just rebuild a SRPM from Fedora (those from F7 to F10 are just a "rpmbuild --rebuild"). Now the Wine maintainer needs to just accept that patch ;) bye michael
Re: [3/4] d3dx9: Implement D3DXAssembleShader function, really basic shader assembler.
2009/12/29 Matteo Bruni : > I added the fake parser for two reasons: because I have to initialize > an asm_parser structure anyway in the create__parser functions > (and I prefer to use parser_fake for the unimplemented shader versions > instead of parser_vs_3) and I want to avoid having some tests for the > other shader versions pass "by chance" inside the todo_wine (this is > accomplished by the asmparser_end_fake function... probably this can > be seen as a hack). > Agreed on the structure fields. > Can't you just let the parser fail when it encounters an unsupported shader version? That's more or less what happens anyway, but I don't think you have to wait until after parsing all the instructions for that.
Re: [3/4] d3dx9: Implement D3DXAssembleShader function, really basic shader assembler.
2009/12/29 Alexandre Julliard : > Matteo Bruni writes: > >> + >> +%option reentrant bison-bridge > > These won't work on old flex versions, and will get you in trouble with > the flex police (aka Michael Stefaniuc ;-) > Alexandre, now you have another person on your side against RHEL 5 and old flex versions :) Joking aside, that's not a big issue as wpp is not reentrant anyway and I'm using the mutex to allow a single call at a time to D3DXAssembleShader. Just a question: as now I have to keep the parser context in a global structure defined in asmshader.y, how should I give access to it from asmshader.l? Do you prefer to give visibility to the global struct from asmshader.l or to create a getter (like a struct asm_parser *asm_get_context(void) function)?
Re: [3/4] d3dx9: Implement D3DXAssembleShader function, really basic shader assembler.
2009/12/29 Henri Verbeet : > 2009/12/28 Matteo Bruni : >> > Why do you need the fake parser? Can't you just not support those > shader versions yet? There's also (in general) not much of a point in > adding structure fields that aren't used yet. > I added the fake parser for two reasons: because I have to initialize an asm_parser structure anyway in the create__parser functions (and I prefer to use parser_fake for the unimplemented shader versions instead of parser_vs_3) and I want to avoid having some tests for the other shader versions pass "by chance" inside the todo_wine (this is accomplished by the asmparser_end_fake function... probably this can be seen as a hack). Agreed on the structure fields. >> +/* This file needs the original d3d9 definitions. The bwriter ones >> + * aren't useable because they are wine-internal things. We're writing >> + * d3d8/9 shaders here, so we need the d3d9 definitions (which are >> + * equal to the d3d8 ones) >> + */ > This doesn't seem to match what the code actually does. That #include is only used from the next patch. I'll move the include and the comment in the right place (maybe rephrasing it somewhat). > >> +/* Debug utility routines. Some are not reentrant, check asmutils.c */ > Same as above. Yep, that's not true anymore, courtesy of wine_dbg_sprintf. > As for splitting things up, I think it's ok to e.g. add the > pre-processor first, and just return E_NOTIMPL from assemble_shader(). > You mean a patch which adds only the first half of the D3DXAssembleShader implementation (returning just after the preprocessing)? That seems reasonable. Btw, ok for your other points also.
Re: [3/4] d3dx9: Implement D3DXAssembleShader function, really basic shader assembler.
Matteo Bruni writes: > + > +%option reentrant bison-bridge These won't work on old flex versions, and will get you in trouble with the flex police (aka Michael Stefaniuc ;-) -- Alexandre Julliard julli...@winehq.org
Re: [3/4] d3dx9: Implement D3DXAssembleShader function, really basic shader assembler.
2009/12/28 Matteo Bruni :
>
Why do you need the fake parser? Can't you just not support those
shader versions yet? There's also (in general) not much of a point in
adding structure fields that aren't used yet.
> +/* This file needs the original d3d9 definitions. The bwriter ones
> + * aren't useable because they are wine-internal things. We're writing
> + * d3d8/9 shaders here, so we need the d3d9 definitions (which are
> + * equal to the d3d8 ones)
> + */
This doesn't seem to match what the code actually does.
> +/* Debug utility routines. Some are not reentrant, check asmutils.c */
Same as above.
> +const char *debug_print_dstreg(const struct shader_reg *reg, shader_type st)
> {
> +return wine_dbg_sprintf("%s", get_regname(reg, st));
> +}
> +
> +const char *debug_print_srcreg(const struct shader_reg *reg, shader_type st)
> {
> +switch(reg->srcmod) {
> +case BWRITERSPSM_NONE:
> +return wine_dbg_sprintf("%s", get_regname(reg, st));
> +}
> +return "Unknown modifier";
> +}
"return get_regname(reg, st);" should work at least as well.
> +/* Mutex used to guarantee a single invocation
> + of the D3DXAssembleShader function (or its variants) at a time.
> + This is needed as wpp isn't thread-safe */
> +extern CRITICAL_SECTION wpp_mutex;
It's probably easier to just statically initialize the critical
section in shader.c.
As for splitting things up, I think it's ok to e.g. add the
pre-processor first, and just return E_NOTIMPL from assemble_shader().
