[Mesa-dev] [PATCH 05/15] i965/fs: Stop relying on param_size in assign_constant_locations

[Jason Ekstrand]

On Thu, Dec 10, 2015 at 1:12 AM, Michael Schellenberger Costa
<mschellenbergercosta at googlemail.com> wrote:
> Hi,
>
> Am 10.12.2015 um 05:23 schrieb Jason Ekstrand:
>> Now that we have MOV_INDIRECT opcodes, we have all of the size information
>> we need directly in the opcode.  With a little restructuring of the
>> algorithm used in assign_constant_locations we don't need param_size
>> anymore.  The big thing to watch out for now, however, is that you can have
>> two ranges overlap where neither contains the other.  In order to deal with
>> this, we make the first pass just flag what needs pulling and handle
>> assigning pull constant locations until later.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 44 ++++++++++++++----------------------
>>  1 file changed, 17 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 786c5fb..1add656 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -1920,14 +1920,12 @@ fs_visitor::assign_constant_locations()
>>     if (dispatch_width != 8)
>>        return;
>>
>> -   unsigned int num_pull_constants = 0;
>> -
>> -   pull_constant_loc = ralloc_array(mem_ctx, int, uniforms);
>> -   memset(pull_constant_loc, -1, sizeof(pull_constant_loc[0]) * uniforms);
>> -
>>     bool is_live[uniforms];
>>     memset(is_live, 0, sizeof(is_live));
>>
>> +   bool needs_pull[uniforms];
>> +   memset(needs_pull, 0, sizeof(is_live));
> While it is valid, could you make this sizeof(needs_pull) to be safe?
> Michael

Yes, that's what we want.  I made and fixed this mistake several times
while working on another patch, but I guess it leaked into this one.
I'll get that fixed.

>> +
>>     /* First, we walk through the instructions and do two things:
>>      *
>>      *  1) Figure out which uniforms are live.
>> @@ -1943,20 +1941,15 @@ fs_visitor::assign_constant_locations()
>>           if (inst->src[i].file != UNIFORM)
>>              continue;
>>
>> -         if (inst->opcode == SHADER_OPCODE_MOV_INDIRECT && i == 0) {
>> -            int uniform = inst->src[0].nr;
>> +         int constant_nr = inst->src[i].nr + inst->src[i].reg_offset;
>>
>> -            /* If this array isn't already present in the pull constant buffer,
>> -             * add it.
>> -             */
>> -            if (pull_constant_loc[uniform] == -1) {
>> -               assert(param_size[uniform]);
>> -               for (int j = 0; j < param_size[uniform]; j++)
>> -                  pull_constant_loc[uniform + j] = num_pull_constants++;
>> +         if (inst->opcode == SHADER_OPCODE_MOV_INDIRECT && i == 0) {
>> +            for (unsigned j = 0; j < inst->src[2].ud / 4; j++) {
>> +               is_live[constant_nr + j] = true;
>> +               needs_pull[constant_nr + j] = true;
>>              }
>>           } else {
>>              /* Mark the the one accessed uniform as live */
>> -            int constant_nr = inst->src[i].nr + inst->src[i].reg_offset;
>>              if (constant_nr >= 0 && constant_nr < (int) uniforms)
>>                 is_live[constant_nr] = true;
>>           }
>> @@ -1973,26 +1966,23 @@ fs_visitor::assign_constant_locations()
>>      */
>>     unsigned int max_push_components = 16 * 8;
>>     unsigned int num_push_constants = 0;
>> +   unsigned int num_pull_constants = 0;
>>
>>     push_constant_loc = ralloc_array(mem_ctx, int, uniforms);
>> +   pull_constant_loc = ralloc_array(mem_ctx, int, uniforms);
>>
>>     for (unsigned int i = 0; i < uniforms; i++) {
>> -      if (!is_live[i] || pull_constant_loc[i] != -1) {
>> -         /* This UNIFORM register is either dead, or has already been demoted
>> -          * to a pull const.  Mark it as no longer living in the param[] array.
>> -          */
>> -         push_constant_loc[i] = -1;
>> +      push_constant_loc[i] = -1;
>> +      pull_constant_loc[i] = -1;
>> +
>> +      if (!is_live[i])
>>           continue;
>> -      }
>>
>> -      if (num_push_constants < max_push_components) {
>> -         /* Retain as a push constant.  Record the location in the params[]
>> -          * array.
>> -          */
>> +      if (!needs_pull[i] && num_push_constants < max_push_components) {
>> +         /* Retain as a push constant */
>>           push_constant_loc[i] = num_push_constants++;
>>        } else {
>> -         /* Demote to a pull constant. */
>> -         push_constant_loc[i] = -1;
>> +         /* We have to pull it */
>>           pull_constant_loc[i] = num_pull_constants++;
>>        }
>>     }
>>