2016-03-28 9:20 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> --- Changes - Changed opcode function to take double arguments and return double
Good catch.
+static struct op_info pres_op_info[] = +{ + {0x000, "nop", 0, 0, NULL }, /* PRESHADER_OP_NOP */ + {0x100, "mov", 1, 0, pres_mov}, /* PRESHADER_OP_MOV */ +};
I think this can be const.
+struct d3dx_pres_ins +{ + enum pres_ops op; + /* first input argument is scalar, + scalar component is propagated */ + BOOL scalar_op; + unsigned int component_count; + struct d3dx_pres_operand inputs[3];
The maximum number of input operands could use a #define. Not a huge deal though.
+static void regstore_free_tables(struct d3dx_regstore *rs) +{ + unsigned int i; + + for (i = PRES_REGTAB_FIRST; i <= PRES_REGTAB_LAST; ++i) + { + if (rs->tables[i]) + HeapFree(GetProcessHeap(), 0, rs->tables[i]); + if (rs->table_value_set[i]) + HeapFree(GetProcessHeap(), 0, rs->table_value_set[i]); + } +}
You don't need to check for non-NULL, HeapFree() handles NULL arguments just fine.
+static unsigned int *find_bytecode_comment(unsigned int *ptr, unsigned int fourcc) +{ + while ((*ptr & 0xffff) == 0xfffe) + { + if (*(ptr + 1) == fourcc) + return ptr + 2; + ptr += 1 + (*ptr >> 16); + } + return NULL; +}
This is probably paranoid but in theory a broken or malicious effect could send you rummaging outside of the bytecode buffer so all the offsets should be checked. I don't think it's critical in practice i.e. I'll probably sleep just fine if you decide to ignore this issue...
+static unsigned int *parse_pres_arg(unsigned int *ptr, struct d3dx_pres_operand *opr) +{ + static const enum pres_reg_tables reg_table[8] = + { + PRES_REGTAB_COUNT, PRES_REGTAB_IMMED, PRES_REGTAB_CONST, PRES_REGTAB_COUNT, + PRES_REGTAB_OCONST, PRES_REGTAB_OBCONST, PRES_REGTAB_OICONST, PRES_REGTAB_REG
Matter of taste, so feel free to ignore this, but PRES_REGTAB_REG sounds a bit vague to me. I'd go with something like PRES_REGTAB_TEMP.
+ if (input_count != pres_op_info[i].input_count) + { + FIXME("Actual input args %u, expected %u, instruction %s.\n", input_count, + pres_op_info[i].input_count, pres_op_info[i].mnem); + return NULL; + } + for (i = 0; i < input_count; ++i) + { + ptr = parse_pres_arg(ptr, &ins->inputs[i]); + if (!ptr) + return NULL; + }
Another pretty paranoid potential check, here you could verify that input_count is < ARRAY_SIZE(ins->inputs). Of course that could only fail if we encounter some instruction with more than 3 components AND we update pres_op_info BUT we forget to update struct d3dx_pres_ins, but I don't think it would hurt to be safe...
+ for (i = 0; i < pres->ins_count; ++i) + { + unsigned int table; + + for (j = 0; j < pres_op_info[pres->ins[i].op].input_count; ++j) + { + table = pres->ins[i].inputs[j].table; + max_offset = get_reg_offset(table, pres->ins[i].inputs[j].offset + pres->ins[i].component_count - 1); + pres->regs.table_sizes[table] = max(pres->regs.table_sizes[table], max_offset + 1); + } + table = pres->ins[i].output.table; + max_offset = get_reg_offset(table, pres->ins[i].output.offset + pres->ins[i].component_count - 1); + pres->regs.table_sizes[table] = max(pres->regs.table_sizes[table], max_offset + 1); + } + update_table_sizes_consts(pres->regs.table_sizes, &pres->inputs);
It probably makes sense to add a small update_table_size() helper (or similar), the "same" 3 lines are repeated in the loop for the input operands, for the output operand and in update_table_sizes_consts().
+ for (i = PRES_REGTAB_IMMED + 1; i <= PRES_REGTAB_LAST; ++i)
It depends on what is coming up next but maybe it would make sense to add an enum for "PRES_REGTAB_IMMED + 1" (incidentally, PRES_REGTAB_FIRST is never used in this patch series).
+ { + TRACE("table_sizes[%u] %u.\n", i, peval->pres.regs.table_sizes[i]);
That trace looks a bit like a debugging leftover.
+ if (FAILED(regstore_alloc_table(&peval->pres.regs, i))) + { + hr = E_OUTOFMEMORY; + goto err_out; + } + } + + if (TRACE_ON(d3dx)) + dump_bytecode(byte_code, byte_code_size); + + *peval_out = peval; + TRACE("retval %u, *peval_out %p.\n", D3D_OK, *peval_out);
Same with this. This one in particular doesn't seem worth keeping.
+ return D3D_OK; + +err_out: + FIXME("Error creating parameter evaluator.\n"); + d3dx_free_param_eval(peval); *peval_out = NULL; - return E_NOTIMPL; + TRACE("retval %u, *peval_out %p.\n", hr, *peval_out); + return hr; +}
You could print the HRESULT in the FIXME (e.g. FIXME("Error creating parameter evaluator, returning %#x.\n", hr)). Tracing the value of *peval_out doesn't seem particularly worthwhile.
+static void d3dx_free_const_tab(struct d3dx_const_tab *ctab) +{ + if (ctab->inputs) + HeapFree(GetProcessHeap(), 0, ctab->inputs); + if (ctab->inputs_param) + HeapFree(GetProcessHeap(), 0, ctab->inputs_param); + if (ctab->ctab) + ID3DXConstantTable_Release(ctab->ctab); +} + +static void d3dx_free_preshader(struct d3dx_preshader *pres) +{ + if (pres->ins) + HeapFree(GetProcessHeap(), 0, pres->ins); + + regstore_free_tables(&pres->regs); + d3dx_free_const_tab(&pres->inputs); }
You can drop all those ifs (I think there are a few more of them in the middle of the patch I haven't specifically mentioned).