[Mesa-dev] [PATCH 17/21] glsl: Make ir_variable::num_state_slots and ir_variable::state_slots private
Tapani
tapani.palli at intel.com
Wed May 28 19:57:27 PDT 2014
On 05/28/2014 10:20 PM, Ian Romanick wrote:
> On 05/28/2014 07:22 AM, Tapani wrote:
>> On 05/28/2014 05:49 AM, Ian Romanick wrote:
>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>
>>> Also move num_state_slots inside ir_variable_data for better packing.
>>>
>>> The payoff for this will come in a few more patches.
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> looks good to me, also I've rebased my cache code on top of these
>> particular changes and internal api introduced for state slots works fine;
>>
>> Reviewed-by: Tapani Pälli <tapani.palli at intel.com>
> Is that for just this patch, this and the preceeding patches in the
> series, the whole series, or something else? :)
Sorry it's just this one for now, I deep dived here first as this one
caused me some changes in my code :) I'll be reading though the other
changes too.
>>> ---
>>> src/glsl/builtin_variables.cpp | 5 +--
>>> src/glsl/ir.h | 56
>>> ++++++++++++++++++--------
>>> src/glsl/ir_clone.cpp | 13 ++----
>>> src/glsl/ir_memory_usage.cpp | 5 ++-
>>> src/glsl/linker.cpp | 7 ++--
>>> src/mesa/drivers/dri/i965/brw_fs.cpp | 6 +--
>>> src/mesa/drivers/dri/i965/brw_shader.cpp | 6 +--
>>> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 6 +--
>>> src/mesa/program/ir_to_mesa.cpp | 14 +++----
>>> src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 14 +++----
>>> 10 files changed, 75 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/src/glsl/builtin_variables.cpp
>>> b/src/glsl/builtin_variables.cpp
>>> index 1461953..5878fbf 100644
>>> --- a/src/glsl/builtin_variables.cpp
>>> +++ b/src/glsl/builtin_variables.cpp
>>> @@ -489,12 +489,9 @@ builtin_variable_generator::add_uniform(const
>>> glsl_type *type,
>>> &_mesa_builtin_uniform_desc[i];
>>> const unsigned array_count = type->is_array() ? type->length : 1;
>>> - uni->num_state_slots = array_count * statevar->num_elements;
>>> ir_state_slot *slots =
>>> - ralloc_array(uni, ir_state_slot, uni->num_state_slots);
>>> -
>>> - uni->state_slots = slots;
>>> + uni->allocate_state_slots(array_count * statevar->num_elements);
>>> for (unsigned a = 0; a < array_count; a++) {
>>> for (unsigned j = 0; j < statevar->num_elements; j++) {
>>> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
>>> index bfd790e..ab9f27b 100644
>>> --- a/src/glsl/ir.h
>>> +++ b/src/glsl/ir.h
>>> @@ -538,6 +538,32 @@ public:
>>> return this->max_ifc_array_access;
>>> }
>>> + inline unsigned get_num_state_slots() const
>>> + {
>>> + return this->data._num_state_slots;
>>> + }
>>> +
>>> + inline void set_num_state_slots(unsigned n)
>>> + {
>>> + this->data._num_state_slots = n;
>>> + }
>>> +
>>> + inline ir_state_slot *get_state_slots()
>>> + {
>>> + return this->state_slots;
>>> + }
>>> +
>>> + inline ir_state_slot *allocate_state_slots(unsigned n)
>>> + {
>>> + this->state_slots = ralloc_array(this, ir_state_slot, n);
>>> + this->data._num_state_slots = 0;
>>> +
>>> + if (this->state_slots != NULL)
>>> + this->data._num_state_slots = n;
>>> +
>>> + return this->state_slots;
>>> + }
>>> +
>>> /**
>>> * Enable emitting extension warnings for this variable
>>> */
>>> @@ -723,6 +749,10 @@ public:
>>> /** Image internal format if specified explicitly, otherwise
>>> GL_NONE. */
>>> uint16_t image_format;
>>> + private:
>>> + unsigned _num_state_slots; /**< Number of state slots used */
>>> +
>>> + public:
>>> /**
>>> * Storage location of the base of this variable
>>> *
>>> @@ -771,22 +801,6 @@ public:
>>> } data;
>>> /**
>>> - * Built-in state that backs this uniform
>>> - *
>>> - * Once set at variable creation, \c state_slots must remain
>>> invariant.
>>> - * This is because, ideally, this array would be shared by all
>>> clones of
>>> - * this variable in the IR tree. In other words, we'd really like
>>> for it
>>> - * to be a fly-weight.
>>> - *
>>> - * If the variable is not a uniform, \c num_state_slots will be
>>> zero and
>>> - * \c state_slots will be \c NULL.
>>> - */
>>> - /*@{*/
>>> - unsigned num_state_slots; /**< Number of state slots used */
>>> - ir_state_slot *state_slots; /**< State descriptors. */
>>> - /*@}*/
>>> -
>>> - /**
>>> * Value assigned in the initializer of a variable declared "const"
>>> */
>>> ir_constant *constant_value;
>>> @@ -818,6 +832,16 @@ private:
>>> unsigned *max_ifc_array_access;
>>> /**
>>> + * Built-in state that backs this uniform
>>> + *
>>> + * Once set at variable creation, \c state_slots must remain
>>> invariant.
>>> + *
>>> + * If the variable is not a uniform, \c _num_state_slots will be
>>> zero and
>>> + * \c state_slots will be \c NULL.
>>> + */
>>> + ir_state_slot *state_slots;
>>> +
>>> + /**
>>> * For variables that are in an interface block or are an
>>> instance of an
>>> * interface block, this is the \c GLSL_TYPE_INTERFACE type for
>>> that block.
>>> *
>>> diff --git a/src/glsl/ir_clone.cpp b/src/glsl/ir_clone.cpp
>>> index d594529..0cd35f0 100644
>>> --- a/src/glsl/ir_clone.cpp
>>> +++ b/src/glsl/ir_clone.cpp
>>> @@ -53,15 +53,10 @@ ir_variable::clone(void *mem_ctx, struct
>>> hash_table *ht) const
>>> memcpy(&var->data, &this->data, sizeof(var->data));
>>> - var->num_state_slots = this->num_state_slots;
>>> - if (this->state_slots) {
>>> - /* FINISHME: This really wants to use something like
>>> talloc_reference, but
>>> - * FINISHME: ralloc doesn't have any similar function.
>>> - */
>>> - var->state_slots = ralloc_array(var, ir_state_slot,
>>> - this->num_state_slots);
>>> - memcpy(var->state_slots, this->state_slots,
>>> - sizeof(this->state_slots[0]) * var->num_state_slots);
>>> + if (this->get_state_slots()) {
>>> + ir_state_slot *s =
>>> var->allocate_state_slots(this->get_num_state_slots());
>>> + memcpy(s, this->get_state_slots(),
>>> + sizeof(s[0]) * var->get_num_state_slots());
>>> }
>>> if (this->constant_value)
>>> diff --git a/src/glsl/ir_memory_usage.cpp b/src/glsl/ir_memory_usage.cpp
>>> index 4918824..f122635 100644
>>> --- a/src/glsl/ir_memory_usage.cpp
>>> +++ b/src/glsl/ir_memory_usage.cpp
>>> @@ -59,8 +59,9 @@ ir_memory_usage::visit(ir_variable *ir)
>>> {
>>> this->s.variable_usage += sizeof(*ir);
>>> - if (ir->state_slots != NULL)
>>> - this->s.variable_usage += (sizeof(ir_state_slot) *
>>> ir->num_state_slots)
>>> + if (ir->get_num_state_slots() != 0)
>>> + this->s.variable_usage +=
>>> + (sizeof(ir_state_slot) * ir->get_num_state_slots())
>>> + ralloc_header_size;
>>> if (ir->name != (char *) ir->padding)
>>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>>> index 4489d9a..b730fb7 100644
>>> --- a/src/glsl/linker.cpp
>>> +++ b/src/glsl/linker.cpp
>>> @@ -1699,9 +1699,10 @@ update_array_sizes(struct gl_shader_program *prog)
>>> * Determine the number of slots per array element by
>>> dividing by
>>> * the old (total) size.
>>> */
>>> - if (var->num_state_slots > 0) {
>>> - var->num_state_slots = (size + 1)
>>> - * (var->num_state_slots / var->type->length);
>>> + const unsigned num_slots = var->get_num_state_slots();
>>> + if (num_slots > 0) {
>>> + var->set_num_state_slots((size + 1)
>>> + * (num_slots /
>>> var->type->length));
>>> }
>>> var->type =
>>> glsl_type::get_array_instance(var->type->fields.array,
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> index 741040b..8fee50c 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> @@ -999,10 +999,10 @@ fs_visitor::setup_uniform_values(ir_variable *ir)
>>> void
>>> fs_visitor::setup_builtin_uniform_values(ir_variable *ir)
>>> {
>>> - const ir_state_slot *const slots = ir->state_slots;
>>> - assert(ir->state_slots != NULL);
>>> + const ir_state_slot *const slots = ir->get_state_slots();
>>> + assert(slots != NULL);
>>> - for (unsigned int i = 0; i < ir->num_state_slots; i++) {
>>> + for (unsigned int i = 0; i < ir->get_num_state_slots(); i++) {
>>> /* This state reference has already been setup by ir_to_mesa,
>>> but we'll
>>> * get the same index back here.
>>> */
>>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp
>>> b/src/mesa/drivers/dri/i965/brw_shader.cpp
>>> index 714c603..684a1f4 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>>> @@ -219,10 +219,10 @@ brw_link_shader(struct gl_context *ctx, struct
>>> gl_shader_program *shProg)
>>> || (strncmp(var->name, "gl_", 3) != 0))
>>> continue;
>>> - const ir_state_slot *const slots = var->state_slots;
>>> - assert(var->state_slots != NULL);
>>> + const ir_state_slot *const slots = var->get_state_slots();
>>> + assert(slots != NULL);
>>> - for (unsigned int i = 0; i < var->num_state_slots; i++) {
>>> + for (unsigned int i = 0; i < var->get_num_state_slots(); i++) {
>>> _mesa_add_state_reference(prog->Parameters,
>>> (gl_state_index *) slots[i].tokens);
>>> }
>>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>> b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>> index d72c47c..a7719c2 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>> @@ -721,10 +721,10 @@ vec4_visitor::setup_uniform_clipplane_values()
>>> void
>>> vec4_visitor::setup_builtin_uniform_values(ir_variable *ir)
>>> {
>>> - const ir_state_slot *const slots = ir->state_slots;
>>> - assert(ir->state_slots != NULL);
>>> + const ir_state_slot *const slots = ir->get_state_slots();
>>> + assert(slots != NULL);
>>> - for (unsigned int i = 0; i < ir->num_state_slots; i++) {
>>> + for (unsigned int i = 0; i < ir->get_num_state_slots(); i++) {
>>> /* This state reference has already been setup by ir_to_mesa,
>>> * but we'll get the same index back here. We can reference
>>> * ParameterValues directly, since unlike brw_fs.cpp, we never
>>> diff --git a/src/mesa/program/ir_to_mesa.cpp
>>> b/src/mesa/program/ir_to_mesa.cpp
>>> index 6112449..831ca41 100644
>>> --- a/src/mesa/program/ir_to_mesa.cpp
>>> +++ b/src/mesa/program/ir_to_mesa.cpp
>>> @@ -687,8 +687,8 @@ ir_to_mesa_visitor::visit(ir_variable *ir)
>>> if (ir->data.mode == ir_var_uniform && strncmp(ir->name, "gl_",
>>> 3) == 0) {
>>> unsigned int i;
>>> - const ir_state_slot *const slots = ir->state_slots;
>>> - assert(ir->state_slots != NULL);
>>> + const ir_state_slot *const slots = ir->get_state_slots();
>>> + assert(slots != NULL);
>>> /* Check if this statevar's setup in the STATE file exactly
>>> * matches how we'll want to reference it as a
>>> @@ -696,7 +696,7 @@ ir_to_mesa_visitor::visit(ir_variable *ir)
>>> * temporary storage and hope that it'll get copy-propagated
>>> * out.
>>> */
>>> - for (i = 0; i < ir->num_state_slots; i++) {
>>> + for (i = 0; i < ir->get_num_state_slots(); i++) {
>>> if (slots[i].swizzle != SWIZZLE_XYZW) {
>>> break;
>>> }
>>> @@ -704,7 +704,7 @@ ir_to_mesa_visitor::visit(ir_variable *ir)
>>> variable_storage *storage;
>>> dst_reg dst;
>>> - if (i == ir->num_state_slots) {
>>> + if (i == ir->get_num_state_slots()) {
>>> /* We'll set the index later. */
>>> storage = new(mem_ctx) variable_storage(ir, PROGRAM_STATE_VAR,
>>> -1);
>>> this->variables.push_tail(storage);
>>> @@ -715,7 +715,7 @@ ir_to_mesa_visitor::visit(ir_variable *ir)
>>> * of the type. However, this had better match the number of
>>> state
>>> * elements that we're going to copy into the new temporary.
>>> */
>>> - assert((int) ir->num_state_slots == type_size(ir->type));
>>> + assert((int) ir->get_num_state_slots() == type_size(ir->type));
>>> storage = new(mem_ctx) variable_storage(ir, PROGRAM_TEMPORARY,
>>> this->next_temp);
>>> @@ -726,7 +726,7 @@ ir_to_mesa_visitor::visit(ir_variable *ir)
>>> }
>>> - for (unsigned int i = 0; i < ir->num_state_slots; i++) {
>>> + for (unsigned int i = 0; i < ir->get_num_state_slots(); i++) {
>>> int index = _mesa_add_state_reference(this->prog->Parameters,
>>> (gl_state_index *)slots[i].tokens);
>>> @@ -746,7 +746,7 @@ ir_to_mesa_visitor::visit(ir_variable *ir)
>>> }
>>> if (storage->file == PROGRAM_TEMPORARY &&
>>> - dst.index != storage->index + (int) ir->num_state_slots) {
>>> + dst.index != storage->index + (int) ir->get_num_state_slots()) {
>>> linker_error(this->shader_program,
>>> "failed to load builtin uniform `%s' "
>>> "(%d/%d regs loaded)\n",
>>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> index 9d98f5b..4dea96a 100644
>>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> @@ -1085,8 +1085,8 @@ glsl_to_tgsi_visitor::visit(ir_variable *ir)
>>> if (ir->data.mode == ir_var_uniform && strncmp(ir->name, "gl_",
>>> 3) == 0) {
>>> unsigned int i;
>>> - const ir_state_slot *const slots = ir->state_slots;
>>> - assert(ir->state_slots != NULL);
>>> + const ir_state_slot *const slots = ir->get_state_slots();
>>> + assert(slots != NULL);
>>> /* Check if this statevar's setup in the STATE file exactly
>>> * matches how we'll want to reference it as a
>>> @@ -1094,7 +1094,7 @@ glsl_to_tgsi_visitor::visit(ir_variable *ir)
>>> * temporary storage and hope that it'll get copy-propagated
>>> * out.
>>> */
>>> - for (i = 0; i < ir->num_state_slots; i++) {
>>> + for (i = 0; i < ir->get_num_state_slots(); i++) {
>>> if (slots[i].swizzle != SWIZZLE_XYZW) {
>>> break;
>>> }
>>> @@ -1102,7 +1102,7 @@ glsl_to_tgsi_visitor::visit(ir_variable *ir)
>>> variable_storage *storage;
>>> st_dst_reg dst;
>>> - if (i == ir->num_state_slots) {
>>> + if (i == ir->get_num_state_slots()) {
>>> /* We'll set the index later. */
>>> storage = new(mem_ctx) variable_storage(ir,
>>> PROGRAM_STATE_VAR, -1);
>>> this->variables.push_tail(storage);
>>> @@ -1113,7 +1113,7 @@ glsl_to_tgsi_visitor::visit(ir_variable *ir)
>>> * of the type. However, this had better match the number
>>> of state
>>> * elements that we're going to copy into the new temporary.
>>> */
>>> - assert((int) ir->num_state_slots == type_size(ir->type));
>>> + assert((int) ir->get_num_state_slots() == type_size(ir->type));
>>> dst = st_dst_reg(get_temp(ir->type));
>>> @@ -1123,7 +1123,7 @@ glsl_to_tgsi_visitor::visit(ir_variable *ir)
>>> }
>>> - for (unsigned int i = 0; i < ir->num_state_slots; i++) {
>>> + for (unsigned int i = 0; i < ir->get_num_state_slots(); i++) {
>>> int index = _mesa_add_state_reference(this->prog->Parameters,
>>> (gl_state_index *)slots[i].tokens);
>>> @@ -1148,7 +1148,7 @@ glsl_to_tgsi_visitor::visit(ir_variable *ir)
>>> }
>>> if (storage->file == PROGRAM_TEMPORARY &&
>>> - dst.index != storage->index + (int) ir->num_state_slots) {
>>> + dst.index != storage->index + (int)
>>> ir->get_num_state_slots()) {
>>> fail_link(this->shader_program,
>>> "failed to load builtin uniform `%s' (%d/%d regs
>>> loaded)\n",
>>> ir->name, dst.index - storage->index,
More information about the mesa-dev
mailing list