On Mon, May 4, 2020 at 10:04 PM Zebediah Figura z.figura12@gmail.com wrote:
Signed-off-by: Zebediah Figura zfigura@codeweavers.com
One potential change to this patch is to always store an offset. Currently NULL is used to signify an offset of 0, partly to save space, and partly because otherwise new_var_deref() would need a bit of restructuring. But always using an offset would simplify some code.
Yeah, that might be a nice followup. Or, it feels like one, without having actually touched the code at all...
dlls/d3dcompiler_43/d3dcompiler_private.h | 25 +--- dlls/d3dcompiler_43/hlsl.y | 143 +++++++++++++--------- dlls/d3dcompiler_43/utils.c | 76 ++---------- 3 files changed, 97 insertions(+), 147 deletions(-)
diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h index 387e932a6cc..983057d543c 100644 --- a/dlls/d3dcompiler_43/d3dcompiler_private.h +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h @@ -841,30 +841,11 @@ struct hlsl_ir_swizzle DWORD swizzle; };
-enum hlsl_ir_deref_type -{
- HLSL_IR_DEREF_VAR,
- HLSL_IR_DEREF_ARRAY,
- HLSL_IR_DEREF_RECORD,
-};
struct hlsl_deref {
- enum hlsl_ir_deref_type type;
- union
- {
struct hlsl_ir_var *var;struct{struct hlsl_ir_node *array;struct hlsl_ir_node *index;} array;struct{struct hlsl_ir_node *record;struct hlsl_struct_field *field;} record;- } v;
- struct hlsl_ir_var *var;
- struct hlsl_ir_node *offset;
- DWORD writemask;
I don't like moving the writemask into struct hlsl_deref. It's something that makes sense only for an lvalue / assignment / STORE. There are a few options here. Maybe turn struct hlsl_deref into struct hlsl_lvalue and use it only in struct hlsl_ir_assignment (i.e. folding the other use into struct hlsl_ir_deref while getting rid of the writemask)? Do we lose a lot by "splitting" struct hlsl_deref?
};
struct hlsl_ir_deref diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y index cc868757a26..24157c8fdc9 100644 --- a/dlls/d3dcompiler_43/hlsl.y +++ b/dlls/d3dcompiler_43/hlsl.y @@ -538,15 +538,29 @@ static struct hlsl_ir_assignment *make_simple_assignment(struct hlsl_ir_var *lhs return NULL;
init_node(&assign->node, HLSL_IR_ASSIGNMENT, rhs->data_type, rhs->loc);
- assign->lhs.type = HLSL_IR_DEREF_VAR;
- assign->lhs.v.var = lhs;
- assign->lhs.var = lhs; assign->rhs = rhs; if (type_is_single_reg(lhs->data_type))
assign->writemask = (1 << lhs->data_type->dimx) - 1;
assign->lhs.writemask = assign->writemask = (1 << lhs->data_type->dimx) - 1;return assign;
}
+static struct hlsl_ir_constant *new_uint_constant(unsigned int n, const struct source_location loc) +{
- struct hlsl_type *type;
- struct hlsl_ir_constant *c;
- if (!(type = new_hlsl_type(d3dcompiler_strdup("uint"), HLSL_CLASS_SCALAR, HLSL_TYPE_UINT, 1, 1)))
return NULL;
En passant: I was thinking that we could do something to avoid redundantly creating hlsl_types over and over, at least for the "basic" types. Probably something very simple, like storing each scalar type in an array, would cover a lot already.
- if (!(c = d3dcompiler_alloc(sizeof(*c))))
return NULL;- init_node(&c->node, HLSL_IR_CONSTANT, type, loc);
- c->v.value.u[0] = n;
- return c;
+}
WRT memory usage, in particular when always storing the offset: we could allocate only the required portion of the whole struct hlsl_ir_constant i.e. something along the lines of "d3dcompiler_alloc(FIELD_OFFSET(struct hlsl_ir_constant, v.value.u[1]))". If the "tricky" allocation happens in a single place, like a new_constant() helper or some-such, maybe it won't be too ugly... BTW I'm not sure that the rest of the compiler is ready for this kind of thing. Let's call it another "nice to have at some point".
static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct source_location loc) { struct hlsl_ir_deref *deref = d3dcompiler_alloc(sizeof(*deref)); @@ -557,60 +571,76 @@ static struct hlsl_ir_deref *new_var_deref(struct hlsl_ir_var *var, const struct return NULL; } init_node(&deref->node, HLSL_IR_DEREF, var->data_type, loc);
- deref->src.type = HLSL_IR_DEREF_VAR;
- deref->src.v.var = var;
- deref->src.var = var;
- if (type_is_single_reg(var->data_type))
deref->src.writemask = (1 << var->data_type->dimx) - 1;}
-static struct hlsl_ir_node *get_var_deref(struct hlsl_ir_node *node) +static struct hlsl_ir_deref *new_deref(struct hlsl_ir_node *var_node, struct hlsl_ir_node *offset,
struct hlsl_type *data_type, const struct source_location loc){
- struct hlsl_ir_assignment *assign;
- struct hlsl_ir_node *add = NULL; struct hlsl_ir_deref *deref; struct hlsl_ir_var *var;
char name[27];
if (node->type == HLSL_IR_DEREF)
return node;
- if (var_node->type == HLSL_IR_DEREF)
- {
const struct hlsl_deref *src = &deref_from_node(var_node)->src;
- sprintf(name, "<deref-%p>", node);
- if (!(var = new_synthetic_var(name, node->data_type, node->loc)))
return NULL;
var = src->var;if (src->offset){if (!(add = new_binary_expr(HLSL_IR_BINOP_ADD, src->offset, offset, loc)))return NULL;list_add_after(&offset->entry, &add->entry);offset = add;}- }
- else
- {
struct hlsl_ir_assignment *assign;char name[27];sprintf(name, "<deref-%p>", var_node);if (!(var = new_synthetic_var(name, var_node->data_type, var_node->loc)))return NULL;
- TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(node->type));
TRACE("Synthesized variable %p for %s node.\n", var, debug_node_type(var_node->type));
- if (!(assign = make_simple_assignment(var, node)))
return NULL;- list_add_after(&node->entry, &assign->node.entry);
if (!(assign = make_simple_assignment(var, var_node)))return NULL;
- if (!(deref = new_var_deref(var, var->loc)))
list_add_after(&var_node->entry, &assign->node.entry);- }
- if (!(deref = d3dcompiler_alloc(sizeof(*deref)))) return NULL;
- list_add_after(&assign->node.entry, &deref->node.entry);
- return &deref->node;
- init_node(&deref->node, HLSL_IR_DEREF, data_type, loc);
- deref->src.var = var;
- deref->src.offset = offset;
- if (type_is_single_reg(data_type))
deref->src.writemask = (1 << data_type->dimx) - 1;- list_add_after(&offset->entry, &deref->node.entry);
- return deref;
}
How bad is it to avoid creating "extra" deref nodes? Mostly curious, it's okay to leave dead code around.