[Mesa-dev] [PATCH 13/21] i965/fs: Combine assign_constant_locations and move_uniform_array_access_to_pull_constants

[Jason Ekstrand]

On Mon, Aug 24, 2015 at 4:48 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, August 19, 2015 10:45:48 PM Jason Ekstrand wrote:
>> The comment above move_uniform_array_access_to_pull_constants was
>> completely bogus because it has nothing to do with lowering instructions.
>> Instead, it's assiging locations of pull constants.
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 38 +++++++++---------------------------
>>  src/mesa/drivers/dri/i965/brw_fs.h   |  1 -
>>  2 files changed, 9 insertions(+), 30 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 68bcbd0..b4003e0 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -1766,21 +1766,19 @@ fs_visitor::compact_virtual_grfs()
>>     return progress;
>>  }
>>
>> -/*
>> - * Implements array access of uniforms by inserting a
>> - * PULL_CONSTANT_LOAD instruction.
>> +/**
>> + * Assign UNIFORM file registers to either push constants or pull constants.
>>   *
>> - * Unlike temporary GRF array access (where we don't support it due to
>> - * the difficulty of doing relative addressing on instruction
>> - * destinations), we could potentially do array access of uniforms
>> - * that were loaded in GRF space as push constants.  In real-world
>> - * usage we've seen, though, the arrays being used are always larger
>> - * than we could load as push constants, so just always move all
>> - * uniform array access out to a pull constant buffer.
>
> It might make sense to leave a comment (for now) saying that we demote
> all indirect addressing of uniform arrays to pull constants.
>
> (Admittedly, that comment would be removed at the end of your patch
> series, but depending how much we merge now vs. later...*shrug*)

Sure.  I can throw one in.

>> + * We allow a fragment shader to have more than the specified minimum
>> + * maximum number of fragment shader uniform components (64).  If
>> + * there are too many of these, they'd fill up all of register space.
>> + * So, this will push some of them out to the pull constant buffer and
>> + * update the program to load them.
>>   */
>>  void
>> -fs_visitor::move_uniform_array_access_to_pull_constants()
>> +fs_visitor::assign_constant_locations()
>>  {
>> +   /* Only the first compile (SIMD8 mode) gets to decide on locations. */
>>     if (dispatch_width != 8)
>>        return;
>>
>> @@ -1817,23 +1815,6 @@ fs_visitor::move_uniform_array_access_to_pull_constants()
>>           }
>>        }
>>     }
>> -}
>> -
>> -/**
>> - * Assign UNIFORM file registers to either push constants or pull constants.
>> - *
>> - * We allow a fragment shader to have more than the specified minimum
>> - * maximum number of fragment shader uniform components (64).  If
>> - * there are too many of these, they'd fill up all of register space.
>> - * So, this will push some of them out to the pull constant buffer and
>> - * update the program to load them.
>> - */
>> -void
>> -fs_visitor::assign_constant_locations()
>> -{
>> -   /* Only the first compile (SIMD8 mode) gets to decide on locations. */
>> -   if (dispatch_width != 8)
>> -      return;
>>
>>     /* Find which UNIFORM registers are still in use. */
>>     bool is_live[uniforms];
>> @@ -4805,7 +4786,6 @@ fs_visitor::optimize()
>>
>>     split_virtual_grfs();
>>
>> -   move_uniform_array_access_to_pull_constants();
>>     assign_constant_locations();
>>     demote_pull_constants();
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>> index 6bca762..31f39fe 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -146,7 +146,6 @@ public:
>>     void spill_reg(int spill_reg);
>>     void split_virtual_grfs();
>>     bool compact_virtual_grfs();
>> -   void move_uniform_array_access_to_pull_constants();
>>     void assign_constant_locations();
>>     void demote_pull_constants();
>>     void invalidate_live_intervals();
>>