[Mesa-dev] [PATCH 1/2] i965: Allocate vec4_visitor's uniform_size and uniform_vector_size arrays dynamically.

[Kenneth Graunke]

On 11/22/2013 11:27 AM, Ian Romanick wrote:
> On 11/22/2013 12:09 AM, Petri Latvala wrote:
>> Signed-off-by: Petri Latvala <petri.latvala at intel.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_vec4.cpp            |  5 +++--
>>  src/mesa/drivers/dri/i965/brw_vec4.h              | 14 +++++++++++---
>>  src/mesa/drivers/dri/i965/brw_vec4_gs.c           |  2 +-
>>  src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp | 12 +++++++-----
>>  src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.h   |  6 ++++--
>>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp    |  7 ++++++-
>>  src/mesa/drivers/dri/i965/brw_vec4_vs_visitor.cpp |  6 ++++--
>>  src/mesa/drivers/dri/i965/brw_vs.c                |  2 +-
>>  src/mesa/drivers/dri/i965/brw_vs.h                |  6 ++++--
>>  9 files changed, 41 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> index 73f91a0..3da1b4d 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> @@ -1558,7 +1558,8 @@ brw_vs_emit(struct brw_context *brw,
>>              struct brw_vs_compile *c,
>>              struct brw_vs_prog_data *prog_data,
>>              void *mem_ctx,
>> -            unsigned *final_assembly_size)
>> +            unsigned *final_assembly_size,
>> +            unsigned param_count)
>>  {
>>     bool start_busy = false;
>>     float start_time = 0;
>> @@ -1585,7 +1586,7 @@ brw_vs_emit(struct brw_context *brw,
>>        }
>>     }
>>  
>> -   vec4_vs_visitor v(brw, c, prog_data, prog, shader, mem_ctx);
>> +   vec4_vs_visitor v(brw, c, prog_data, prog, shader, mem_ctx, param_count);
>>     if (!v.run()) {
>>        if (prog) {
>>           prog->LinkStatus = false;
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
>> index 5cec9f9..552ca55 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
>> @@ -231,7 +231,8 @@ public:
>>  		struct brw_shader *shader,
>>  		void *mem_ctx,
>>                  bool debug_flag,
>> -                bool no_spills);
>> +                bool no_spills,
>> +                unsigned param_count);
>>     ~vec4_visitor();
>>  
>>     dst_reg dst_null_f()
>> @@ -325,8 +326,9 @@ public:
>>      */
>>     dst_reg output_reg[BRW_VARYING_SLOT_COUNT];
>>     const char *output_reg_annotation[BRW_VARYING_SLOT_COUNT];
>> -   int uniform_size[MAX_UNIFORMS];
>> -   int uniform_vector_size[MAX_UNIFORMS];
>> +   unsigned uniform_param_count;
>> +   int* uniform_size;
>> +   int* uniform_vector_size;
>>     int uniforms;
>>  
>>     src_reg shader_start_time;
>> @@ -546,6 +548,12 @@ private:
>>      * If true, then register allocation should fail instead of spilling.
>>      */
>>     const bool no_spills;
>> +
>> +   /*
>> +    * Make noncopyable.
>> +    */
>> +   vec4_visitor(const vec4_visitor&);
>> +   vec4_visitor& operator=(const vec4_visitor&);
> 
> I think this should be a separate patch with it's own justification.
> I'm also not sure it's strictly necessary.  I'm not a C++ expert (and
> most people on the project aren't either), so bear with me a bit.  The
> usual reason to make a class non-copyable is because the default
> copy-constructor will only make a shallow copy of pointer fields.  This
> can lead to either double-frees or dereferencing freed memory.  However,
> since we're using ralloc, and the first construction of a vec4_visitor
> object will out-live any possible copies, neither of these situations
> can occur.  Is that a fair assessment?
> 
> Now... we probably should do this anyway... and there are a lot of
> classes in the i965 back-end where this should occur.  I don't know if
> we want to make a macro to generate this boiler-plate code or if we just
> want to manually add it to every class.  Perhaps Ken, Paul, or Curro
> will have an opinion...

Or you could just be sensible and just not make copies of things where
it makes no sense to do so...

I would drop this hunk...it's unrelated and unnecessary.

--Ken