[Mesa-dev] [PATCH 1/2] ac: fix broken elimination of duplicated VS exports

[Nicolai Hähnle]

On 08.05.2017 18:38, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> The renumbering code didn't take into account that multiple VS exports
> can have the same PARAM index. This also significantly simplifies
> the renumbering. Thankfully, we have piglits for this:
>
>     spec at arb_gpu_shader5@arb_gpu_shader5-interpolateatcentroid-packing
>     spec at glsl-1.50@execution at interface-blocks-complex-vs-fs
>
> Reported by Michel Dänzer.

That does look cleaner.

Fixes: b08715499e61 ("ac: eliminate duplicated VS exports")
Cc: mesa-stable at lists.freedesktop.org
Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

Also R-b for patch #2.

> ---
>  src/amd/common/ac_llvm_build.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/src/amd/common/ac_llvm_build.c b/src/amd/common/ac_llvm_build.c
> index 9853d17..87a1fb7 100644
> --- a/src/amd/common/ac_llvm_build.c
> +++ b/src/amd/common/ac_llvm_build.c
> @@ -1401,37 +1401,37 @@ void ac_optimize_vs_outputs(struct ac_llvm_context *ctx,
>  				exports.exp[exports.num++] = exp;
>  			}
>  		}
>  		bb = LLVMGetNextBasicBlock(bb);
>  	}
>
>  	/* Remove holes in export memory due to removed PARAM exports.
>  	 * This is done by renumbering all PARAM exports.
>  	 */
>  	if (removed_any) {
> -		uint8_t current_offset[VARYING_SLOT_MAX];
> -		unsigned new_count = 0;
> +		uint8_t old_offset[VARYING_SLOT_MAX];
>  		unsigned out, i;
>
>  		/* Make a copy of the offsets. We need the old version while
>  		 * we are modifying some of them. */
> -		memcpy(current_offset, vs_output_param_offset,
> -		       sizeof(current_offset));
> +		memcpy(old_offset, vs_output_param_offset,
> +		       sizeof(old_offset));
>
>  		for (i = 0; i < exports.num; i++) {
>  			unsigned offset = exports.exp[i].offset;
>
> +			/* Update vs_output_param_offset. Multiple outputs can
> +			 * have the same offset.
> +			 */
>  			for (out = 0; out < num_outputs; out++) {
> -				if (current_offset[out] != offset)
> -					continue;
> -
> -				LLVMSetOperand(exports.exp[i].inst, AC_EXP_TARGET,
> -					       LLVMConstInt(ctx->i32,
> -							    V_008DFC_SQ_EXP_PARAM + new_count, 0));
> -				vs_output_param_offset[out] = new_count;
> -				new_count++;
> -				break;
> +				if (old_offset[out] == offset)
> +					vs_output_param_offset[out] = i;
>  			}
> +
> +			/* Change the PARAM offset in the instruction. */
> +			LLVMSetOperand(exports.exp[i].inst, AC_EXP_TARGET,
> +				       LLVMConstInt(ctx->i32,
> +						    V_008DFC_SQ_EXP_PARAM + i, 0));
>  		}
> -		*num_param_exports = new_count;
> +		*num_param_exports = exports.num;
>  	}
>  }
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.