[Mesa-dev] [PATCH 2/2] i965: Handle nested uniform array indexing

[Kenneth Graunke]

On Tuesday, November 18, 2014 09:15:06 PM Chris Forbes wrote:
> When converting a uniform array reference to a pull constant load, the
> `reladdr` expression itself may have its own `reladdr`, arbitrarily
> deeply. This arises from expressions like:
> 
>    a[b[x]]     where a, b are uniform arrays (or lowered const arrays),
>                and x is not a constant.
> 
> Just iterate the lowering to pull constants until we stop seeing these
> nested. For most shaders, there will be only one pass through this loop.
> 
> Fixes the piglit test:
> tests/spec/glsl-1.20/linker/double-indirect-1.shader_test
> 
> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
> Cc: "10.3 10.4" <mesa-stable at lists.freedesktop.org>
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 66 
+++++++++++++++-----------
>  1 file changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp 
b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index af7ca0c..22a6fb9 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -3354,6 +3354,7 @@ 
vec4_visitor::move_uniform_array_access_to_pull_constants()
>  {
>     int pull_constant_loc[this->uniforms];
>     memset(pull_constant_loc, -1, sizeof(pull_constant_loc));
> +   bool nested_reladdr;
>  
>     /* Walk through and find array access of uniforms.  Put a copy of that
>      * uniform in the pull constant buffer.
> @@ -3361,44 +3362,51 @@ 
vec4_visitor::move_uniform_array_access_to_pull_constants()
>      * Note that we don't move constant-indexed accesses to arrays.  No
>      * testing has been done of the performance impact of this choice.
>      */
> -   foreach_block_and_inst_safe(block, vec4_instruction, inst, cfg) {
> -      for (int i = 0 ; i < 3; i++) {
> -	 if (inst->src[i].file != UNIFORM || !inst->src[i].reladdr)
> -	    continue;
> +   do {
> +      nested_reladdr = false;

Yikes.  Good find on the bug.

Wrapping it in a loop like this is definitely nice and simple, which is great 
for stable branches.  It might be worth adding a TODO comment for doing it 
more efficiently someday:

/* TODO: We could refactor this and avoid doing N passes over the instruction
 * stream (where N is the reladdr nesting depth).
 */

I think this is fine, though, since the reladdr depth is probably 0 or 1...and 
we just found our first case of 2 today...

I think you need an equivalent patch for the FS.

As is,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141118/4aebebd3/attachment.sig>