New subject: [v4 2/6] d3dx9: add preshaders to effect.

18 Mar 2016


      2016-03-17 12:59 GMT+01:00 Paul Gofman gofmanp@gmail.com:
...
+enum PRES_REG_TABLES
+{

PRES_REGTAB_NONE,
PRES_REGTAB_IMMED, /* immediate double constants from CLIT */
PRES_REGTAB_CONST,
PRES_REGTAB_VERTEX, /* not used */
PRES_REGTAB_OCONST,
PRES_REGTAB_OBCONST,
PRES_REGTAB_OICONST,
PRES_REGTAB_REG,
PRES_REGTAB_MAX,
PRES_REGTAB_FIRST = PRES_REGTAB_IMMED,
PRES_REGTAB_LAST = PRES_REGTAB_REG,

+};
You can drop PRES_REGTAB_NONE unless it's going to be used.
...
+++ b/dlls/d3dx9_36/preshader.c
@@ -0,0 +1,627 @@
+/*


FX preshader parsing & execution, setting shader constants



from effect parameters.









It looks like that kind of general file description is omitted in
newer source files. Maybe drop it? I don't particularly care though.
...

/* TODO: use double precision for x64 arch */
{sizeof(float),  4, PRES_VT_FLOAT }  /* PRES_REGTAB_REG */

Nitpick, just mention "64 bit" or something, we don't want to make all
the non-Intel archs angry ;)
...
+#define PRES_VS_BITS_PER_WORD (sizeof(unsigned int) * 8)
What does the _VS_ part of the name mean?
I'm also a bit thrown off by the _WORD part, mostly because in Win32
WORD is a 16 bit while DWORD is 32 bit and unsigned int is also 32
bit. Maybe it's just me, I don't know.
...
+static void dump_shader_bytecode(void *data, unsigned int size)
+{

unsigned int *words = (unsigned int *)data;

Same here. I guess you could call it "bytecode" or such.
...
+static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc)
+{

while ((*ptr & 0xFFFF) == 0xFFFE)

A couple leftover uppercase hexadecimal constants.
...

hr = D3DXGetShaderConstantTable(byte_code, &ctab);
if (FAILED(hr) || !ctab)
{
   TRACE("Could not get CTAB data, hr %#x.\n", hr);


   /* returning OK is not a typo */


   return D3D_OK;


}

Sorry, back to that comment again. I guess it would be better to spell
out why that's the intended return value instead (e.g. "Not returning
an error, shaders without a CTAB are valid.").
...
+static void dump_arg(struct d3dx_regstore *rs, struct d3dx_pres_operand *arg, int component_count)
+{

static const char *xyzw_str = "xyzw";
unsigned int i, table;

table = arg->table;
if (table == PRES_REGTAB_IMMED)
{
   TRACE("(");


   for (i = 0; i < component_count; ++i)


       TRACE(i < component_count - 1 ? "%lf, " : "%lf",


               ((double *)rs->tables[PRES_REGTAB_IMMED])[arg->offset + i]);



%lf is not right, %f already expects a double. I'd just use %.16e
(which has the added benefit of showing something different from 0 in
the "precision" test from the previous patch). Another option would be
to use the same format used by MS when disassembling shaders, although
this seems to be a case where it's worth diverging.
...
+static void dump_ins(struct d3dx_regstore *rs, struct d3dx_pres_ins *ins)
+{

unsigned int i;

TRACE("    %s ", pres_op_info[ins->op].mnem);
dump_arg(rs, &ins->output, pres_op_info[ins->op].func_all_comps ? 1 : ins->component_count);
for (i = 0; i < pres_op_info[ins->op].input_count; ++i)
{
   TRACE(",");



With a blank after the comma it would be prettier.
There is still some inconsistent formatting (e.g. "== NULL" vs "!",
uppercase hexadecimals, cnt vs count).
A possible idea to split this patch further: what if you move the
addition of (most of) the contents of the various functions in
preshader.c to a separate patch? That is, make a patch which adds the
various calls to d3dx_create_param_eval(), d3dx_free_param_eval() and
similar (i.e. most if not all the changes to effect.c) but only
introduces a skeleton implementation of those functions in
preshader.c. Those are actually fleshed out only in the next patch.
Many of the new preshader-related definitions can probably also go
into the second patch.
The traces of the disassembled preshader can also potentially go to a
separate patch (leaving only dump_shader_bytecode() in the initial
implementation - btw, I think it should be called
dump_preshader_bytecode() given the renaming of the other stuff).

    

Re: [v4 2/6] d3dx9: add preshaders to effect.