[Mesa-dev] [PATCH 11/18] radeonsi: don't emit unnecessary NULL exports for unbound targets

[Nicolai Hähnle]

On 05.02.2016 14:20, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> ---
>   src/gallium/drivers/radeonsi/si_shader.c | 63 +++++++++++++++++++++++++-------
>   1 file changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
> index bd45d4a..18e4ab9 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -2298,13 +2298,30 @@ static void si_export_mrt_color(struct lp_build_tgsi_context *bld_base,
>   	/* If last_cbuf > 0, FS_COLOR0_WRITES_ALL_CBUFS is true. */
>   	if (index == 0 &&
>   	    si_shader_ctx->shader->key.ps.last_cbuf > 0) {
> -		for (int c = 1; c <= si_shader_ctx->shader->key.ps.last_cbuf; c++) {
> +		LLVMValueRef args[8][9];

This declaration shadows the top-level args array. I suggest to turn 
this into an if-else where the else part covers the last_cbuf == 0 case. 
IMHO that also makes the control flow clearer.

Also, while we're at it, I believe that the index == 0 is not needed 
when last_cbuf > 0, and it could be clearer to omit it. At least in 
GLSL, gl_FragColor and gl_FragData are mutually exclusive. I don't feel 
strongly about it though.

> +		int c, last = -1;
> +
> +		/* Get the export arguments, also find out what the last one is. */
> +		for (c = 0; c <= si_shader_ctx->shader->key.ps.last_cbuf; c++) {
>   			si_llvm_init_export_args(bld_base, color,
> -						 V_008DFC_SQ_EXP_MRT + c, args);
> +						 V_008DFC_SQ_EXP_MRT + c, args[c]);
> +			if (args[c][0] != bld_base->uint_bld.zero)
> +				last = c;
> +		}
> +
> +		/* Emit all exports. */
> +		for (c = 0; c <= si_shader_ctx->shader->key.ps.last_cbuf; c++) {
> +			if (is_last && last == c) {
> +				args[c][1] = bld_base->uint_bld.one; /* whether the EXEC mask is valid */
> +				args[c][2] = bld_base->uint_bld.one; /* DONE bit */
> +			} else if (args[c][0] == bld_base->uint_bld.zero)
> +				continue; /* unnecessary NULL export */
> +
>   			lp_build_intrinsic(base->gallivm->builder, "llvm.SI.export",
>   					   LLVMVoidTypeInContext(base->gallivm->context),
> -					   args, 9, 0);
> +					   args[c], 9, 0);
>   		}
> +		return;
>   	}
>
>   	/* Export */
> @@ -2313,7 +2330,9 @@ static void si_export_mrt_color(struct lp_build_tgsi_context *bld_base,
>   	if (is_last) {
>   		args[1] = bld_base->uint_bld.one; /* whether the EXEC mask is valid */
>   		args[2] = bld_base->uint_bld.one; /* DONE bit */
> -	}
> +	} else if (args[0] == bld_base->uint_bld.zero)
> +		return; /* unnecessary NULL export */
> +
>   	lp_build_intrinsic(base->gallivm->builder, "llvm.SI.export",
>   			   LLVMVoidTypeInContext(base->gallivm->context),
>   			   args, 9, 0);
> @@ -2351,19 +2370,37 @@ static void si_llvm_emit_fs_epilogue(struct lp_build_tgsi_context * bld_base)
>   	int last_color_export = -1;
>   	int i;
>
> -	/* If there are no outputs, add a dummy export. */
> -	if (!info->num_outputs) {
> -		si_export_null(bld_base);
> -		return;
> -	}
> -
>   	/* Determine the last export. If MRTZ is present, it's always last.
>   	 * Otherwise, find the last color export.
>   	 */
> -	if (!info->writes_z && !info->writes_stencil && !info->writes_samplemask)
> -		for (i = 0; i < info->num_outputs; i++)
> -			if (info->output_semantic_name[i] == TGSI_SEMANTIC_COLOR)
> +	if (!info->writes_z && !info->writes_stencil && !info->writes_samplemask) {
> +		unsigned spi_format = shader->key.ps.spi_shader_col_format;
> +
> +		for (i = 0; i < info->num_outputs; i++) {
> +			unsigned index = info->output_semantic_index[i];
> +
> +			if (info->output_semantic_name[i] != TGSI_SEMANTIC_COLOR)
> +				continue;
> +
> +			/* If last_cbuf > 0, FS_COLOR0_WRITES_ALL_CBUFS is true. */
> +			if (index == 0 && shader->key.ps.last_cbuf > 0) {
> +				/* Just set this if any of the colorbuffers are enabled. */
> +				if (spi_format &
> +				    ((1llu << (4 * (shader->key.ps.last_cbuf + 1))) - 1))
> +					last_color_export = i;
> +				continue;
> +			}

Another index == 0 that is probably redundant.

Cheers,
Nicolai

> +
> +			if ((spi_format >> (index * 4)) & 0xf)
>   				last_color_export = i;
> +		}
> +
> +		/* If there are no outputs, export NULL. */
> +		if (last_color_export == -1) {
> +			si_export_null(bld_base);
> +			return;
> +		}
> +	}
>
>   	for (i = 0; i < info->num_outputs; i++) {
>   		unsigned semantic_name = info->output_semantic_name[i];
>