Hi to everybody, I'm sending here the main assembler patch for reviews and suggestions. It is bzipped as it is quite a big patch, but I couldn't find a meaningful way to split it. A quick mile-high overview: the assembler is divided in a parser and a bytecode writer. The parser is made by the flex (asmshader.l) and bison (asmshader.y) source files and the asmparser.c file. The bytecode writer is in bytecodewriter.c, btw, and asmutils.c contains some utility functions used in the assembler. With the asmshader.l SlAssembleShader function the parser processes the shader and returns an intermediate representation of it into the wine_shader struct. Then this struct is passed to the bytecode writer with the SlWriteBytecode function, which produces the assembled shader bytecode. A "point of interest": in the bottom part of asmshader.h there are some definitions borrowed from wined3d. They are used only internally, so they should not be a problem, but I'd like to hear if you think differently (ex. it is better to rename them?).
Thank you, Matteo Bruni
2009/7/19 Matteo Bruni matteo.mystral@gmail.com:
Hi to everybody, I'm sending here the main assembler patch for reviews and suggestions. It is bzipped as it is quite a big patch, but I couldn't find a meaningful way to split it.
It really is much easier to review as separate patches. The part that follows is really only the first thing I noticed.
+const char *debug_src(struct shader_reg *reg, BOOL vs) {
- static char buffer[128];
- memset(buffer, 0, sizeof(buffer));
This really isn't acceptable: - "debug_src" isn't a very good name for something that visible outside the current file - The reg parameter should be const. - Zeroing the entire buffer is quite unnecessary. - Just passing the shader type would be much better than a BOOL parameter "vs" and hoping that anything that isn't a vertex shader is a pixel shader. Even if it happens to be true. - "buffer" has a fixed size. - "buffer" is static. Using wine_dbg_sprintf() would already be much better, but the entire "parsed_shader" setup looks flawed to me. It seems to me that you should handle that by having an appropriately filled struct asmparser_backend, and appending to a proper buffer.
Note that I think your mentor should have caught basic things like this in a much earlier stage.
2009/7/19 Henri Verbeet hverbeet@gmail.com:
2009/7/19 Matteo Bruni matteo.mystral@gmail.com:
Hi to everybody, I'm sending here the main assembler patch for reviews and suggestions. It is bzipped as it is quite a big patch, but I couldn't find a meaningful way to split it.
It really is much easier to review as separate patches. The part that follows is really only the first thing I noticed.
+const char *debug_src(struct shader_reg *reg, BOOL vs) {
- static char buffer[128];
- memset(buffer, 0, sizeof(buffer));
This really isn't acceptable: - "debug_src" isn't a very good name for something that visible outside the current file - The reg parameter should be const. - Zeroing the entire buffer is quite unnecessary. - Just passing the shader type would be much better than a BOOL parameter "vs" and hoping that anything that isn't a vertex shader is a pixel shader. Even if it happens to be true. - "buffer" has a fixed size. - "buffer" is static. Using wine_dbg_sprintf() would already be much better, but the entire "parsed_shader" setup looks flawed to me. It seems to me that you should handle that by having an appropriately filled struct asmparser_backend, and appending to a proper buffer.
Note that I think your mentor should have caught basic things like this in a much earlier stage.
That function, in particular, should really be into asmparser.c and not be visible from outside. Then the wine_dbg_sprintf() function comes really handy in this situation, I didn't know it. Note also that this debug_src function is used just to print trace info during asm parsing, not to generate the intermediate representation or the bytecode. However, I think I've got your point. I'm going to look to other similar mistakes in the code before resending. A question: do you have an idea how I could split this in separate patches? I can think of separating the parser from the bytecode writer, but doesn't seems to me that this would be a real improvement. Adding some shader instructions handling each time (for ex. starting with just shader model 1, then separate patches for the subsequent versions) maybe is better, but the first patch won't be really simpler than this, I believe, as it would be alike this patch but without a bunch of cases/functions.
Thank you, Matteo.
Am Monday 20 July 2009 01:38:35 schrieb Matteo Bruni:
2009/7/19 Henri Verbeet hverbeet@gmail.com:
2009/7/19 Matteo Bruni matteo.mystral@gmail.com:
+const char *debug_src(struct shader_reg *reg, BOOL vs) {
- static char buffer[128];
- memset(buffer, 0, sizeof(buffer));
- "buffer" is static. Using wine_dbg_sprintf() would already be much better, but the entire "parsed_shader" setup looks flawed to me. It seems to me that you should handle that by having an appropriately filled struct asmparser_backend, and appending to a proper buffer.
Once debug_src uses wine_dbg_sprintf you can also change code like this:
- TRACE_(parsed_shader)("add%s%s %s, %s, ", debug_dstmod(mod),
debug_shift(shift), debug_dst(dst, vs),debug_src(src0, vs));- TRACE_(parsed_shader)("%s\n", debug_src(src1, vs));
And write all srcs in one TRACE.
2009/7/20 Matteo Bruni matteo.mystral@gmail.com:
That function, in particular, should really be into asmparser.c and not be visible from outside. Then the wine_dbg_sprintf() function comes really handy in this situation, I didn't know it. Note also that this debug_src function is used just to print trace info during asm parsing, not to generate the intermediate representation or the bytecode.
Yeah, but you can treat the debug output just like another asmparser_backend. That would mean calling the parser twice when debugging, but that should be ok. You can do something similar on the bytecode writing side.
A question: do you have an idea how I could split this in separate patches? I can think of separating the parser from the bytecode writer, but doesn't seems to me that this would be a real improvement. Adding some shader instructions handling each time (for ex. starting with just shader model 1, then separate patches for the subsequent versions) maybe is better, but the first patch won't be really simpler than this, I believe, as it would be alike this patch but without a bunch of cases/functions.
You could probably split it into a patch adding the basic framework and separate patches for each instruction. That also implies that you can wait with adding supporting functions like the various debug functions until they're used for the first time. Thinking about the basic design some more, it's not entirely clear to me why you've got separate asmparser_*() functions for each instruction though. They look largely similar, and I think the differences like e.g. number of source arguments could be handled by an appropriate lookup table.
2009/7/20 Henri Verbeet hverbeet@gmail.com:
2009/7/20 Matteo Bruni matteo.mystral@gmail.com:
That function, in particular, should really be into asmparser.c and not be visible from outside. Then the wine_dbg_sprintf() function comes really handy in this situation, I didn't know it. Note also that this debug_src function is used just to print trace info during asm parsing, not to generate the intermediate representation or the bytecode.
Yeah, but you can treat the debug output just like another asmparser_backend. That would mean calling the parser twice when debugging, but that should be ok. You can do something similar on the bytecode writing side.
I'm not so persuaded on making another backend for debug messages, as in this way the debug things would be separated from the processing code, and could easily happen they don't perfectly match (by mistakes). If you don't feel strongly against, I prefer to just fix the debug_src things as you suggested, without splitting the parsed_shader infrastructure.
A question: do you have an idea how I could split this in separate patches? I can think of separating the parser from the bytecode writer, but doesn't seems to me that this would be a real improvement. Adding some shader instructions handling each time (for ex. starting with just shader model 1, then separate patches for the subsequent versions) maybe is better, but the first patch won't be really simpler than this, I believe, as it would be alike this patch but without a bunch of cases/functions.
You could probably split it into a patch adding the basic framework and separate patches for each instruction. That also implies that you can wait with adding supporting functions like the various debug functions until they're used for the first time.
This needs some work but certainly can be done, and will be done when finally sending the patches to wine-patches.
Thinking about the basic design some more, it's not entirely clear to me why you've got separate asmparser_*() functions for each instruction though. They look largely similar, and I think the differences like e.g. number of source arguments could be handled by an appropriate lookup table.
While in principle this is right, as many functions are indeed very similar, this won't be pretty practically, even excluding that some instructions really need special treatment. The problem is that the different asmparser_* functions have different signatures (0, 1, 2 or 3 src registers, comparison or not, etc). So the generic function should accept all the parameter combinations used by them (leading to a quite long function signature), most of them set to NULL in the function invocation, or it should manage a variable number of arguments. Conversely, this is very much doable in the bytecode writer, as there all the functions have already the same format; I can simply replace the function pointers in the backends to make them point to a single function (excluding the instructions that really need special handling), and the function, using a lookup table or a switch, can convert from the internal instruction opcode to the D3D one.
2009/7/20 Matteo Bruni matteo.mystral@gmail.com:
2009/7/20 Henri Verbeet hverbeet@gmail.com:
Yeah, but you can treat the debug output just like another asmparser_backend. That would mean calling the parser twice when debugging, but that should be ok. You can do something similar on the bytecode writing side.
I'm not so persuaded on making another backend for debug messages, as in this way the debug things would be separated from the processing code, and could easily happen they don't perfectly match (by mistakes).
They don't necessarily have to, the debug code only has to print what the parser read.
If you don't feel strongly against, I prefer to just fix the debug_src things as you suggested, without splitting the parsed_shader infrastructure.
I don't feel strongly about it, but it's probably less trouble in the long run.
Thinking about the basic design some more, it's not entirely clear to me why you've got separate asmparser_*() functions for each instruction though. They look largely similar, and I think the differences like e.g. number of source arguments could be handled by an appropriate lookup table.
While in principle this is right, as many functions are indeed very similar, this won't be pretty practically, even excluding that some instructions really need special treatment. The problem is that the different asmparser_* functions have different signatures (0, 1, 2 or 3 src registers, comparison or not, etc). So the generic function should accept all the parameter combinations used by them (leading to a quite long function signature), most of them set to NULL in the function invocation, or it should manage a variable number of arguments.
Which functions need special treatment? You can pass source arguments as an array + count, that's how they're stored in "struct instruction" anyway. All these callbacks really do is building an instruction and adding them to the array. You may not need a callback for that at all. Of course this will require some changes to the way the parser is setup.
Am Monday 20 July 2009 20:33:44 schrieb Henri Verbeet:
While in principle this is right, as many functions are indeed very similar, this won't be pretty practically, even excluding that some instructions really need special treatment.
One more thing comes to my mind here: The assembler doesn't actually check if the registers for the instruction are valid. You can happily assemble a shader that doesn't make sense, but validation fails. The validation is implemented in d3d9.dll.Direct3DShaderValidatorCreate9, not in the assembler:
For example: vs_2_0 rep c0 endrep mov oPos, c0
$ vsa.exe shader.txt /Fh shader.h (with native d3d9) Microsoft (R) D3DX9 Shader Assembler 9.25 PRIVATE Copyright (C) Microsoft Corporation 2002-2003. All rights reserved.
Z:\dev\shm\shader.txt(2,1): error X5936: rep requires first parameter to be integer constant register (i#).
assembly failed; no code produced
$ vsa.exe shader.txt /Fh shader.h (with builtin d3d9) Microsoft (R) D3DX9 Shader Assembler 9.25 PRIVATE Copyright (C) Microsoft Corporation 2002-2003. All rights reserved.
fixme:d3d9:Direct3DShaderValidatorCreate9 stub assembly succeeded; see shader.h
So while its certainly nice to print a warning if such a thing happens, the assembler doesn't have to catch it, and must not return an error. Its certainly worth testing if this holds true in d3dx9 though, since vsa.exe seems to have its own copy of the assembler.
As I said to Matteo in an IRC conversion, my main intentions for the one-callback-per-instruction setup when I wrote it last year were this sort of error handling(which I later found out doesn't happen in the assembler), and to map all shader versions to a shared format, and then generate any target shader version(ie, convert between two shader versions), which is now moot as well since we put the shader into d3dx9 and strictly stick to the MS API that way.
Am Sunday 19 July 2009 23:01:08 schrieb Henri Verbeet:
+const char *debug_src(struct shader_reg *reg, BOOL vs) {
- static char buffer[128];
- memset(buffer, 0, sizeof(buffer));
This really isn't acceptable:
Note that I think your mentor should have caught basic things like this in a much earlier stage.
Heh, it was me who wrote that specific piece of code in the first place, more than a year ago :-)
-
Henri Verbeet -
Matteo Bruni -
Stefan Dösinger