[Mesa-dev] [PATCH] i965/vec4: Fix liveness analysis with BRW_OPCODE_SEL

[Kenneth Graunke]

On Monday, July 20, 2015 03:14:14 PM Iago Toral Quiroga wrote:
> We only consider a vgrf defined by a given block if the block writes to it
> unconditionally. So far we have been checking this by testing that the
> instruction is not predicated, however, in the case of BRW_OPCODE_SEL,
> the predication is used to select the value to write, not to decide if
> the write is actually done. The consequence of this was increased life
> spans for affected vgrfs, which could lead to additional register pressure.
> 
> Since NIR generates selects for conditional writes this was causing massive
> register pressure in a handful of piglit and dEQP tests that had a large
> number of select operations with the NIR-vec4 backend.
> 
> Fixes the following piglit tests with the NIR-vec4 backend:
> spec/glsl-1.50/execution/variable-indexing/vs-output-array-vec4-index-wr-before-gs
> spec/glsl-1.50/execution/variable-indexing/gs-input-array-vec4-index-rd
> spec/glsl-1.50/execution/variable-indexing/vs-output-array-vec2-index-wr-before-gs
> spec/glsl-1.50/execution/variable-indexing/vs-output-array-vec3-index-wr-before-gs
> spec/glsl-1.50/execution/variable-indexing/vs-output-array-float-index-wr-before-gs
> 
> Fixes the 80 dEQP tests with the NIR-vec4 backend in the following category:
> dEQP-GLES3.functional.ubo.*
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> index 29b4a53..cc688ef 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_live_variables.cpp
> @@ -96,7 +96,8 @@ vec4_live_variables::setup_def_use()
>  	  * are the things that screen off preceding definitions of a
>  	  * variable, and thus qualify for being in def[].
>  	  */
> -	 if (inst->dst.file == GRF && !inst->predicate) {
> +	 if (inst->dst.file == GRF &&
> +	     (!inst->predicate || inst->opcode == BRW_OPCODE_SEL)) {
>              for (unsigned i = 0; i < inst->regs_written; i++) {
>                 for (int c = 0; c < 4; c++) {
>                    if (inst->dst.writemask & (1 << c)) {
> 

Thanks!  I made a similar fix in the FS backend a long time ago;
apparently I forgot to check the vec4 backend.  Good catch.

NIR probably generates a lot more SELs, so it makes sense this would
cause more trouble with that.

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/20150720/cab44b3d/attachment.sig>