D3D shader assembler (round 2)

[Matteo Bruni]

2009/8/10 Stefan Dösinger :
> 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

[Stefan Dösinger]

Am Wednesday 12 August 2009 21:57:36 schrieb Matteo Bruni:
> > 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

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

Fair enough
> > > - 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?

Hmm. Maybe D3DXSIO_*, or WINED3DXSIO_*, but I'll think about a better one. 
Also this applies to many other enums.
> 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?

Aren't they used by the parser as well? Ie, to generate the wineshader 
structure?
> > 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.

Looks ok to me
> 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 :)

I added them after I got fed up with writing HeapAlloc(GetProcessHeap(), 
HEAP_ZERO_MEMORY, size); all the time, and I considered them ugly
(A while ago there was some talk about a wine-wide HeapAlloc wrapper macro 
too)
> 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?

I'd say give wine-patches a try :-) Make sure Alexandre can see how these 
patches will work together with the wpp ones and the actual implementation of 
D3DXAssembleShader

[Henri Verbeet]

2009/8/13 Stefan Dösinger :
> I'd say give wine-patches a try :-) Make sure Alexandre can see how these
> patches will work together with the wpp ones and the actual implementation of
> D3DXAssembleShader

If you do, try to split it up a bit more where possible, it's still a
pretty large patch to review.