[Mesa-dev] [PATCH 17/21] glsl: Make ir_variable::num_state_slots and ir_variable::state_slots private

Ian Romanick idr at freedesktop.org
Wed May 28 12:20:30 PDT 2014


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? :)

>> ---
>>   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