[Mesa-dev] [PATCH] compiler/spirv: set is_shadow for depth comparitor sampling opcodes

[Iago Toral]

On Mon, 2018-04-02 at 09:31 -0700, Jason Ekstrand wrote:
> 
> On April 2, 2018 04:04:49 Iago Toral Quiroga <itoral at igalia.com>
> wrote:
> 
> From the SPIR-V spec, OpTypeImage:
> 
> "Depth is whether or not this image is a depth image. (Note that
> whether or not depth comparisons are actually done is a property of
> the sampling opcode, not of this type declaration.)"
> 
> OpImageSample{Proj}Dref{Explicit,Implicit}Lod sampling opcodes always
> do
> a depth comparison, regardless of the Depth property of the image
> type,
> and as stated in the spec quote, this should be honored. For us, it
> means
> that we always have to handle them as shadow sampling opcodes.
> 
> Fixes crashes in:
> dEQP-
> VK.spirv_assembly.instruction.graphics.image_sampler.depth_property.*
> ---
> src/compiler/spirv/spirv_to_nir.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compiler/spirv/spirv_to_nir.c 
> b/src/compiler/spirv/spirv_to_nir.c
> index 72ab426e80..f968c4d387 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -1908,7 +1908,22 @@ vtn_handle_texture(struct vtn_builder *b,
> SpvOp opcode,
> const struct glsl_type *image_type = sampled.type->type;
> const enum glsl_sampler_dim sampler_dim =
> glsl_get_sampler_dim(image_type);
> const bool is_array = glsl_sampler_type_is_array(image_type);
> -   const bool is_shadow = glsl_sampler_type_is_shadow(image_type);
> +
> +   bool is_shadow;
> +   switch (opcode) {
> +   /* These opcodes are always depth-comparitors regardless of the
> depth
> +    * property of the image
> +    */
> +   case SpvOpImageSampleDrefImplicitLod:
> +   case SpvOpImageSampleProjDrefImplicitLod:
> +   case SpvOpImageSampleDrefExplicitLod:
> +   case SpvOpImageSampleProjDrefExplicitLod:
> +      is_shadow = true;
> +      break;
> +   default:
> +      is_shadow = glsl_sampler_type_is_shadow(image_type);
> 
> Given the sure quote above, should this just be false?

Yes, I think you're right. I have just tested a shadow mapping demo I
have around and verified that GLSLang produces these opcodes for shadow
samplers, so I think it should be safe to do this change. I'll run it
through Jenkins and send a new patch if that doesn't find anything
suspicious.

>   Also, don't we 
> already have a switch for the Dref source that we can use instead of
> making 
> a new one?

Yes, we can use other switches to do this, although we will need to use
a fallthrough, but I guess that's okay.

> 
> +      break;
> +   }
> 
> /* Figure out the base texture operation */
> nir_texop texop;
> --
> 2.14.1
> 
> 
> 
>