[Mesa-dev] [PATCH] ac/nir: Only clamp shadow reference on radeonsi.

[Nicolai Hähnle]

On 23.10.2017 03:32, Bas Nieuwenhuizen wrote:
> Vulkan CTS does not expect the value to be clamped (at least for D32),
> and it makes a differences even though depth is in [0,1], due
> to strict inequalities.
> 
> I couldn't find anything in the Vulkan spec about this, but the test
> seemed to be copied from GL tests and the GL spec only specifies
> clamping for fixed point formats. Hence I expect radeonsi to run into
> this at some point as well, but given that they still have a usecase
> with the Z16->Z32 promotion, I'll leave that for someone else to clean
> up.

radeonsi does this optionally based on an added software-only descriptor 
bit; that change still needs to be added to the NIR path yet.

Cheers,
Nicolai

> 
> This at least fixes radv dEQP-VK.texture.shadow.* on VI.
> 
> Fixes: 0f9e32519bb 'ac/nir: clamp shadow texture comparison value on VI'
> ---
>   src/amd/common/ac_nir_to_llvm.c              | 5 +++--
>   src/amd/common/ac_nir_to_llvm.h              | 1 +
>   src/amd/common/ac_shader_abi.h               | 4 ++++
>   src/gallium/drivers/radeonsi/si_shader_nir.c | 1 +
>   4 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c
> index 5e5a46a21f4..2ddc748b5be 100644
> --- a/src/amd/common/ac_nir_to_llvm.c
> +++ b/src/amd/common/ac_nir_to_llvm.c
> @@ -4652,14 +4652,14 @@ static void visit_tex(struct ac_nir_context *ctx, nir_tex_instr *instr)
>   		LLVMValueRef z = ac_to_float(&ctx->ac,
>   		                             llvm_extract_elem(&ctx->ac, comparator, 0));
>   
> -		/* TC-compatible HTILE promotes Z16 and Z24 to Z32_FLOAT,
> +		/* TC-compatible HTILE on radeonsi promotes Z16 and Z24 to Z32_FLOAT,
>   		 * so the depth comparison value isn't clamped for Z16 and
>   		 * Z24 anymore. Do it manually here.
>   		 *
>   		 * It's unnecessary if the original texture format was
>   		 * Z32_FLOAT, but we don't know that here.
>   		 */
> -		if (ctx->ac.chip_class == VI)
> +		if (ctx->ac.chip_class == VI && ctx->abi->clamp_shadow_reference)
>   			z = ac_build_clamp(&ctx->ac, z);
>   
>   		address[count++] = z;
> @@ -6600,6 +6600,7 @@ LLVMModuleRef ac_translate_nir_to_llvm(LLVMTargetMachineRef tm,
>   	ctx.abi.emit_outputs = handle_shader_outputs_post;
>   	ctx.abi.load_ssbo = radv_load_ssbo;
>   	ctx.abi.load_sampler_desc = radv_get_sampler_desc;
> +	ctx.abi.clamp_shadow_reference = false;
>   
>   	if (shader_count >= 2)
>   		ac_init_exec_full_mask(&ctx.ac);
> diff --git a/src/amd/common/ac_nir_to_llvm.h b/src/amd/common/ac_nir_to_llvm.h
> index 9579aeeefef..1d9ec8ce8b0 100644
> --- a/src/amd/common/ac_nir_to_llvm.h
> +++ b/src/amd/common/ac_nir_to_llvm.h
> @@ -80,6 +80,7 @@ struct ac_nir_compiler_options {
>   	struct ac_shader_variant_key key;
>   	bool unsafe_math;
>   	bool supports_spill;
> +	bool clamp_shadow_reference;
>   	enum radeon_family family;
>   	enum chip_class chip_class;
>   };
> diff --git a/src/amd/common/ac_shader_abi.h b/src/amd/common/ac_shader_abi.h
> index 5f296be0c1f..14517d55700 100644
> --- a/src/amd/common/ac_shader_abi.h
> +++ b/src/amd/common/ac_shader_abi.h
> @@ -88,6 +88,10 @@ struct ac_shader_abi {
>   					  LLVMValueRef index,
>   					  enum ac_descriptor_type desc_type,
>   					  bool image, bool write);
> +
> +	/* Whether to clamp the shadow reference value to [0,1]on VI. Radeonsi currently
> +	 * uses it due to promoting D16 to D32, but radv needs it off. */
> +	bool clamp_shadow_reference;
>   };
>   
>   #endif /* AC_SHADER_ABI_H */
> diff --git a/src/gallium/drivers/radeonsi/si_shader_nir.c b/src/gallium/drivers/radeonsi/si_shader_nir.c
> index a2d175364f8..e186661caf3 100644
> --- a/src/gallium/drivers/radeonsi/si_shader_nir.c
> +++ b/src/gallium/drivers/radeonsi/si_shader_nir.c
> @@ -498,6 +498,7 @@ bool si_nir_build_llvm(struct si_shader_context *ctx, struct nir_shader *nir)
>   
>   	ctx->abi.inputs = &ctx->inputs[0];
>   	ctx->abi.load_sampler_desc = si_nir_load_sampler_desc;
> +	ctx->abi.clamp_shadow_reference = true;
>   
>   	ctx->num_samplers = util_last_bit(info->samplers_declared);
>   	ctx->num_images = util_last_bit(info->images_declared);
> 


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