Re: [Mesa-dev] [PATCH] spirv: Use correct type for sampled images

[Alex Smith]

On 6 January 2018 at 01:03, Jason Ekstrand  wrote:

> On Tue, Nov 7, 2017 at 3:08 AM, Alex Smith 
> wrote:
>
>> Thanks Jason. Can someone push this?
>>
>
> Did you never get push access?
>

I did - this is commit e9eb3c4753e4f56b03d16d8d6f71d49f1e7b97db.

Thanks,
Alex


> --Jason
>
>
>> On 6 November 2017 at 16:21, Jason Ekstrand  wrote:
>>
>>> On Mon, Nov 6, 2017 at 2:37 AM, Alex Smith 
>>> wrote:
>>>
 We should use the result type of the OpSampledImage opcode, rather than
 the type of the underlying image/samplers.

 This resolves an issue when using separate images and shadow samplers
 with glslang. Example:

 layout (...) uniform samplerShadow s0;
 layout (...) uniform texture2D res0;
 ...
 float result = textureLod(sampler2DShadow(res0, s0), uv, 0);

 For this, for the combined OpSampledImage, the type of the base image
 was being used (which does not have the Depth flag set, whereas the
 result type does), therefore it was not being recognised as a shadow
 sampler. This led to the wrong LLVM intrinsics being emitted by RADV.

>>>
>>> Reviewed-by: Jason Ekstrand 
>>>
>>>
 Signed-off-by: Alex Smith 
 Cc: "17.2 17.3" 
 ---
  src/compiler/spirv/spirv_to_nir.c  | 10 --
  src/compiler/spirv/vtn_private.h   |  1 +
  src/compiler/spirv/vtn_variables.c |  1 +
  3 files changed, 6 insertions(+), 6 deletions(-)

 diff --git a/src/compiler/spirv/spirv_to_nir.c
 b/src/compiler/spirv/spirv_to_nir.c
 index 6825e0d6a8..93a515d731 100644
 --- a/src/compiler/spirv/spirv_to_nir.c
 +++ b/src/compiler/spirv/spirv_to_nir.c
 @@ -1490,6 +1490,8 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
 opcode,
struct vtn_value *val =
   vtn_push_value(b, w[2], vtn_value_type_sampled_image);
val->sampled_image = ralloc(b, struct vtn_sampled_image);
 +  val->sampled_image->type =
 + vtn_value(b, w[1], vtn_value_type_type)->type;
val->sampled_image->image =
   vtn_value(b, w[3], vtn_value_type_pointer)->pointer;
val->sampled_image->sampler =
 @@ -1516,16 +1518,12 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
 opcode,
sampled = *sampled_val->sampled_image;
 } else {
assert(sampled_val->value_type == vtn_value_type_pointer);
 +  sampled.type = sampled_val->pointer->type;
sampled.image = NULL;
sampled.sampler = sampled_val->pointer;
 }

 -   const struct glsl_type *image_type;
 -   if (sampled.image) {
 -  image_type = sampled.image->var->var->interface_type;
 -   } else {
 -  image_type = sampled.sampler->var->var->interface_type;
 -   }
 +   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);
 diff --git a/src/compiler/spirv/vtn_private.h
 b/src/compiler/spirv/vtn_private.h
 index 84584620fc..6b4645acc8 100644
 --- a/src/compiler/spirv/vtn_private.h
 +++ b/src/compiler/spirv/vtn_private.h
 @@ -411,6 +411,7 @@ struct vtn_image_pointer {
  };

  struct vtn_sampled_image {
 +   struct vtn_type *type;
 struct vtn_pointer *image; /* Image or array of images */
 struct vtn_pointer *sampler; /* Sampler */
  };
 diff --git a/src/compiler/spirv/vtn_variables.c
 b/src/compiler/spirv/vtn_variables.c
 index 1cf9d597cf..9a69b4f6fc 100644
 --- a/src/compiler/spirv/vtn_variables.c
 +++ b/src/compiler/spirv/vtn_variables.c
 @@ -1805,6 +1805,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp
 opcode,
   struct vtn_value *val =
  vtn_push_value(b, w[2], vtn_value_type_sampled_image);
   val->sampled_image = ralloc(b, struct vtn_sampled_image);
 + val->sampled_image->type = base_val->sampled_image->type;
   val->sampled_image->image =
  vtn_pointer_dereference(b, base_val->sampled_image->image,
 chain);
   val->sampled_image->sampler = base_val->sampled_image->sampl
 er;
 --
 2.13.6


>>>
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] spirv: Use correct type for sampled images

[Jason Ekstrand]

On Tue, Nov 7, 2017 at 3:08 AM, Alex Smith 
wrote:

> Thanks Jason. Can someone push this?
>

Did you never get push access?
--Jason


> On 6 November 2017 at 16:21, Jason Ekstrand  wrote:
>
>> On Mon, Nov 6, 2017 at 2:37 AM, Alex Smith 
>> wrote:
>>
>>> We should use the result type of the OpSampledImage opcode, rather than
>>> the type of the underlying image/samplers.
>>>
>>> This resolves an issue when using separate images and shadow samplers
>>> with glslang. Example:
>>>
>>> layout (...) uniform samplerShadow s0;
>>> layout (...) uniform texture2D res0;
>>> ...
>>> float result = textureLod(sampler2DShadow(res0, s0), uv, 0);
>>>
>>> For this, for the combined OpSampledImage, the type of the base image
>>> was being used (which does not have the Depth flag set, whereas the
>>> result type does), therefore it was not being recognised as a shadow
>>> sampler. This led to the wrong LLVM intrinsics being emitted by RADV.
>>>
>>
>> Reviewed-by: Jason Ekstrand 
>>
>>
>>> Signed-off-by: Alex Smith 
>>> Cc: "17.2 17.3" 
>>> ---
>>>  src/compiler/spirv/spirv_to_nir.c  | 10 --
>>>  src/compiler/spirv/vtn_private.h   |  1 +
>>>  src/compiler/spirv/vtn_variables.c |  1 +
>>>  3 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/compiler/spirv/spirv_to_nir.c
>>> b/src/compiler/spirv/spirv_to_nir.c
>>> index 6825e0d6a8..93a515d731 100644
>>> --- a/src/compiler/spirv/spirv_to_nir.c
>>> +++ b/src/compiler/spirv/spirv_to_nir.c
>>> @@ -1490,6 +1490,8 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
>>> opcode,
>>>struct vtn_value *val =
>>>   vtn_push_value(b, w[2], vtn_value_type_sampled_image);
>>>val->sampled_image = ralloc(b, struct vtn_sampled_image);
>>> +  val->sampled_image->type =
>>> + vtn_value(b, w[1], vtn_value_type_type)->type;
>>>val->sampled_image->image =
>>>   vtn_value(b, w[3], vtn_value_type_pointer)->pointer;
>>>val->sampled_image->sampler =
>>> @@ -1516,16 +1518,12 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
>>> opcode,
>>>sampled = *sampled_val->sampled_image;
>>> } else {
>>>assert(sampled_val->value_type == vtn_value_type_pointer);
>>> +  sampled.type = sampled_val->pointer->type;
>>>sampled.image = NULL;
>>>sampled.sampler = sampled_val->pointer;
>>> }
>>>
>>> -   const struct glsl_type *image_type;
>>> -   if (sampled.image) {
>>> -  image_type = sampled.image->var->var->interface_type;
>>> -   } else {
>>> -  image_type = sampled.sampler->var->var->interface_type;
>>> -   }
>>> +   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);
>>> diff --git a/src/compiler/spirv/vtn_private.h
>>> b/src/compiler/spirv/vtn_private.h
>>> index 84584620fc..6b4645acc8 100644
>>> --- a/src/compiler/spirv/vtn_private.h
>>> +++ b/src/compiler/spirv/vtn_private.h
>>> @@ -411,6 +411,7 @@ struct vtn_image_pointer {
>>>  };
>>>
>>>  struct vtn_sampled_image {
>>> +   struct vtn_type *type;
>>> struct vtn_pointer *image; /* Image or array of images */
>>> struct vtn_pointer *sampler; /* Sampler */
>>>  };
>>> diff --git a/src/compiler/spirv/vtn_variables.c
>>> b/src/compiler/spirv/vtn_variables.c
>>> index 1cf9d597cf..9a69b4f6fc 100644
>>> --- a/src/compiler/spirv/vtn_variables.c
>>> +++ b/src/compiler/spirv/vtn_variables.c
>>> @@ -1805,6 +1805,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp
>>> opcode,
>>>   struct vtn_value *val =
>>>  vtn_push_value(b, w[2], vtn_value_type_sampled_image);
>>>   val->sampled_image = ralloc(b, struct vtn_sampled_image);
>>> + val->sampled_image->type = base_val->sampled_image->type;
>>>   val->sampled_image->image =
>>>  vtn_pointer_dereference(b, base_val->sampled_image->image,
>>> chain);
>>>   val->sampled_image->sampler = base_val->sampled_image->sampl
>>> er;
>>> --
>>> 2.13.6
>>>
>>>
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] spirv: Use correct type for sampled images

[Alex Smith]

Thanks Jason. Can someone push this?

On 6 November 2017 at 16:21, Jason Ekstrand  wrote:

> On Mon, Nov 6, 2017 at 2:37 AM, Alex Smith 
> wrote:
>
>> We should use the result type of the OpSampledImage opcode, rather than
>> the type of the underlying image/samplers.
>>
>> This resolves an issue when using separate images and shadow samplers
>> with glslang. Example:
>>
>> layout (...) uniform samplerShadow s0;
>> layout (...) uniform texture2D res0;
>> ...
>> float result = textureLod(sampler2DShadow(res0, s0), uv, 0);
>>
>> For this, for the combined OpSampledImage, the type of the base image
>> was being used (which does not have the Depth flag set, whereas the
>> result type does), therefore it was not being recognised as a shadow
>> sampler. This led to the wrong LLVM intrinsics being emitted by RADV.
>>
>
> Reviewed-by: Jason Ekstrand 
>
>
>> Signed-off-by: Alex Smith 
>> Cc: "17.2 17.3" 
>> ---
>>  src/compiler/spirv/spirv_to_nir.c  | 10 --
>>  src/compiler/spirv/vtn_private.h   |  1 +
>>  src/compiler/spirv/vtn_variables.c |  1 +
>>  3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/compiler/spirv/spirv_to_nir.c
>> b/src/compiler/spirv/spirv_to_nir.c
>> index 6825e0d6a8..93a515d731 100644
>> --- a/src/compiler/spirv/spirv_to_nir.c
>> +++ b/src/compiler/spirv/spirv_to_nir.c
>> @@ -1490,6 +1490,8 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
>> opcode,
>>struct vtn_value *val =
>>   vtn_push_value(b, w[2], vtn_value_type_sampled_image);
>>val->sampled_image = ralloc(b, struct vtn_sampled_image);
>> +  val->sampled_image->type =
>> + vtn_value(b, w[1], vtn_value_type_type)->type;
>>val->sampled_image->image =
>>   vtn_value(b, w[3], vtn_value_type_pointer)->pointer;
>>val->sampled_image->sampler =
>> @@ -1516,16 +1518,12 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
>> opcode,
>>sampled = *sampled_val->sampled_image;
>> } else {
>>assert(sampled_val->value_type == vtn_value_type_pointer);
>> +  sampled.type = sampled_val->pointer->type;
>>sampled.image = NULL;
>>sampled.sampler = sampled_val->pointer;
>> }
>>
>> -   const struct glsl_type *image_type;
>> -   if (sampled.image) {
>> -  image_type = sampled.image->var->var->interface_type;
>> -   } else {
>> -  image_type = sampled.sampler->var->var->interface_type;
>> -   }
>> +   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);
>> diff --git a/src/compiler/spirv/vtn_private.h
>> b/src/compiler/spirv/vtn_private.h
>> index 84584620fc..6b4645acc8 100644
>> --- a/src/compiler/spirv/vtn_private.h
>> +++ b/src/compiler/spirv/vtn_private.h
>> @@ -411,6 +411,7 @@ struct vtn_image_pointer {
>>  };
>>
>>  struct vtn_sampled_image {
>> +   struct vtn_type *type;
>> struct vtn_pointer *image; /* Image or array of images */
>> struct vtn_pointer *sampler; /* Sampler */
>>  };
>> diff --git a/src/compiler/spirv/vtn_variables.c
>> b/src/compiler/spirv/vtn_variables.c
>> index 1cf9d597cf..9a69b4f6fc 100644
>> --- a/src/compiler/spirv/vtn_variables.c
>> +++ b/src/compiler/spirv/vtn_variables.c
>> @@ -1805,6 +1805,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp
>> opcode,
>>   struct vtn_value *val =
>>  vtn_push_value(b, w[2], vtn_value_type_sampled_image);
>>   val->sampled_image = ralloc(b, struct vtn_sampled_image);
>> + val->sampled_image->type = base_val->sampled_image->type;
>>   val->sampled_image->image =
>>  vtn_pointer_dereference(b, base_val->sampled_image->image,
>> chain);
>>   val->sampled_image->sampler = base_val->sampled_image->sampler;
>> --
>> 2.13.6
>>
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] spirv: Use correct type for sampled images

[Jason Ekstrand]

On Mon, Nov 6, 2017 at 2:37 AM, Alex Smith 
wrote:

> We should use the result type of the OpSampledImage opcode, rather than
> the type of the underlying image/samplers.
>
> This resolves an issue when using separate images and shadow samplers
> with glslang. Example:
>
> layout (...) uniform samplerShadow s0;
> layout (...) uniform texture2D res0;
> ...
> float result = textureLod(sampler2DShadow(res0, s0), uv, 0);
>
> For this, for the combined OpSampledImage, the type of the base image
> was being used (which does not have the Depth flag set, whereas the
> result type does), therefore it was not being recognised as a shadow
> sampler. This led to the wrong LLVM intrinsics being emitted by RADV.
>

Reviewed-by: Jason Ekstrand 


> Signed-off-by: Alex Smith 
> Cc: "17.2 17.3" 
> ---
>  src/compiler/spirv/spirv_to_nir.c  | 10 --
>  src/compiler/spirv/vtn_private.h   |  1 +
>  src/compiler/spirv/vtn_variables.c |  1 +
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> index 6825e0d6a8..93a515d731 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -1490,6 +1490,8 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
> opcode,
>struct vtn_value *val =
>   vtn_push_value(b, w[2], vtn_value_type_sampled_image);
>val->sampled_image = ralloc(b, struct vtn_sampled_image);
> +  val->sampled_image->type =
> + vtn_value(b, w[1], vtn_value_type_type)->type;
>val->sampled_image->image =
>   vtn_value(b, w[3], vtn_value_type_pointer)->pointer;
>val->sampled_image->sampler =
> @@ -1516,16 +1518,12 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp
> opcode,
>sampled = *sampled_val->sampled_image;
> } else {
>assert(sampled_val->value_type == vtn_value_type_pointer);
> +  sampled.type = sampled_val->pointer->type;
>sampled.image = NULL;
>sampled.sampler = sampled_val->pointer;
> }
>
> -   const struct glsl_type *image_type;
> -   if (sampled.image) {
> -  image_type = sampled.image->var->var->interface_type;
> -   } else {
> -  image_type = sampled.sampler->var->var->interface_type;
> -   }
> +   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);
> diff --git a/src/compiler/spirv/vtn_private.h b/src/compiler/spirv/vtn_
> private.h
> index 84584620fc..6b4645acc8 100644
> --- a/src/compiler/spirv/vtn_private.h
> +++ b/src/compiler/spirv/vtn_private.h
> @@ -411,6 +411,7 @@ struct vtn_image_pointer {
>  };
>
>  struct vtn_sampled_image {
> +   struct vtn_type *type;
> struct vtn_pointer *image; /* Image or array of images */
> struct vtn_pointer *sampler; /* Sampler */
>  };
> diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_
> variables.c
> index 1cf9d597cf..9a69b4f6fc 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1805,6 +1805,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp
> opcode,
>   struct vtn_value *val =
>  vtn_push_value(b, w[2], vtn_value_type_sampled_image);
>   val->sampled_image = ralloc(b, struct vtn_sampled_image);
> + val->sampled_image->type = base_val->sampled_image->type;
>   val->sampled_image->image =
>  vtn_pointer_dereference(b, base_val->sampled_image->image,
> chain);
>   val->sampled_image->sampler = base_val->sampled_image->sampler;
> --
> 2.13.6
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] spirv: Use correct type for sampled images

[Alex Smith]

We should use the result type of the OpSampledImage opcode, rather than
the type of the underlying image/samplers.

This resolves an issue when using separate images and shadow samplers
with glslang. Example:

layout (...) uniform samplerShadow s0;
layout (...) uniform texture2D res0;
...
float result = textureLod(sampler2DShadow(res0, s0), uv, 0);

For this, for the combined OpSampledImage, the type of the base image
was being used (which does not have the Depth flag set, whereas the
result type does), therefore it was not being recognised as a shadow
sampler. This led to the wrong LLVM intrinsics being emitted by RADV.

Signed-off-by: Alex Smith 
Cc: "17.2 17.3" 
---
 src/compiler/spirv/spirv_to_nir.c  | 10 --
 src/compiler/spirv/vtn_private.h   |  1 +
 src/compiler/spirv/vtn_variables.c |  1 +
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/compiler/spirv/spirv_to_nir.c 
b/src/compiler/spirv/spirv_to_nir.c
index 6825e0d6a8..93a515d731 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1490,6 +1490,8 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp opcode,
   struct vtn_value *val =
  vtn_push_value(b, w[2], vtn_value_type_sampled_image);
   val->sampled_image = ralloc(b, struct vtn_sampled_image);
+  val->sampled_image->type =
+ vtn_value(b, w[1], vtn_value_type_type)->type;
   val->sampled_image->image =
  vtn_value(b, w[3], vtn_value_type_pointer)->pointer;
   val->sampled_image->sampler =
@@ -1516,16 +1518,12 @@ vtn_handle_texture(struct vtn_builder *b, SpvOp opcode,
   sampled = *sampled_val->sampled_image;
} else {
   assert(sampled_val->value_type == vtn_value_type_pointer);
+  sampled.type = sampled_val->pointer->type;
   sampled.image = NULL;
   sampled.sampler = sampled_val->pointer;
}
 
-   const struct glsl_type *image_type;
-   if (sampled.image) {
-  image_type = sampled.image->var->var->interface_type;
-   } else {
-  image_type = sampled.sampler->var->var->interface_type;
-   }
+   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);
diff --git a/src/compiler/spirv/vtn_private.h b/src/compiler/spirv/vtn_private.h
index 84584620fc..6b4645acc8 100644
--- a/src/compiler/spirv/vtn_private.h
+++ b/src/compiler/spirv/vtn_private.h
@@ -411,6 +411,7 @@ struct vtn_image_pointer {
 };
 
 struct vtn_sampled_image {
+   struct vtn_type *type;
struct vtn_pointer *image; /* Image or array of images */
struct vtn_pointer *sampler; /* Sampler */
 };
diff --git a/src/compiler/spirv/vtn_variables.c 
b/src/compiler/spirv/vtn_variables.c
index 1cf9d597cf..9a69b4f6fc 100644
--- a/src/compiler/spirv/vtn_variables.c
+++ b/src/compiler/spirv/vtn_variables.c
@@ -1805,6 +1805,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode,
  struct vtn_value *val =
 vtn_push_value(b, w[2], vtn_value_type_sampled_image);
  val->sampled_image = ralloc(b, struct vtn_sampled_image);
+ val->sampled_image->type = base_val->sampled_image->type;
  val->sampled_image->image =
 vtn_pointer_dereference(b, base_val->sampled_image->image, chain);
  val->sampled_image->sampler = base_val->sampled_image->sampler;
-- 
2.13.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev