[Mesa-dev] [PATCH 07/29] nir/lower_tex: fix get_zero_or_one() to use sized types

[Samuel Iglesias Gonsálvez]

On 23/03/16 15:56, Jason Ekstrand wrote:
> On Mar 23, 2016 12:32 AM, "Samuel Iglesias Gonsálvez"
> <siglesias at igalia.com> wrote:
>> 
>> On 21/03/16 23:40, Jason Ekstrand wrote:
>>> On Mon, Mar 21, 2016 at 3:39 PM, Jason Ekstrand
>>> <jason at jlekstrand.net> wrote:
>>> 
>>>> 
>>>> 
>>>> On Mon, Mar 21, 2016 at 5:05 AM, Samuel Iglesias Gonsálvez < 
>>>> siglesias at igalia.com> wrote:
>>>> 
>>>>> From: Iago Toral Quiroga <itoral at igalia.com>
>>>>> 
>>>>> v2 (Sam): - Use helper to get type size from nir_alu_type
>>>>> enum. --- src/compiler/nir/nir_lower_tex.c | 3 ++- 1 file
>>>>> changed, 2 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/src/compiler/nir/nir_lower_tex.c 
>>>>> b/src/compiler/nir/nir_lower_tex.c index 4999603..a58385a
>>>>> 100644 --- a/src/compiler/nir/nir_lower_tex.c +++
>>>>> b/src/compiler/nir/nir_lower_tex.c @@ -226,7 +226,8 @@
>>>>> get_zero_or_one(nir_builder *b, nir_alu_type type, uint8_t
>>>>> swizzle_val) v.u32[0] = v.u32[1] = v.u32[2] = v.u32[3] =
>>>>> 0; } else { assert(swizzle_val == 5); -      if (type ==
>>>>> nir_type_float) +
>>>>> assert(nir_alu_type_get_type_size(type) < 64); +      if
>>>>> (type == nir_type_float32)
>>>>> 
>>>> 
>>>> This seems dangerious.  It switches a check from checking for
>>>> float to checking for float32.  At the particular point in
>>>> the git history where this patch lands, one of these checks
>>>> is broken.  I'm guessing this actually breaks things that get
>>>> fixed later when we switch glsl_to_nir
> to
>>>> giving us sized types.
>>>> 
>> 
>> Right, Connor modified glsl_to_nir to add sized types to the
>> destination type. Actually, these patches should come after it,
>> we reordered them when preparing this series for submission.
>> 
>>>> If we're going to change tex instructions to use sized types,
>>>> we need
> to
>>>> do it all in one big patch and it will have to touch all
>>>> three drivers again.
>>>> 
>>> 
>>> One more thought:  Tex instructions already have a bit size
>>> provided by
> the
>>> destination so I don't see a need for having it be sized at
>>> all.  At the end of the day it doesn't matter since we either
>>> don't have a size or
> we do
>>> have a size and it's required to match.
>>> 
>>> 
>> 
>> Yeah, right. Currently brw_glsl_base_type_for_nir_type() and 
>> brw_type_for_nir_type() manage unsized types but our idea was to
>> support only sized types in the driver at the end of the fp64
>> changes. See this patch [0].
> 
> That seems reasonable as long as we don't break anything along the
> way.
> 
>> If we are not going to change tex instructions to use sized
>> types, then we just need to get rid of patch [0] because
>> otherwise it would break tex instructions emission.
>> 
>> What do you think?
>> 
>> If we agree with keeping unsized types for tex instructions, I
>> will remove these patches from the series:
>> 
>> nir/lower_tex: fix get_zero_or_one() to use sized types 
>> nir/lower_tex: fix get_texture_size() to use sized types 
>> program/nir: include bit-size information
>> 
>> Delete the respective ir_visitor::visit(ir_texture *ir) hunk
>> from:
>> 
>> nir/glsl_to_nir: support doubles
> 
> I don't really care too much much one way or the other.  I think my
> mild preference would be to keep them unsized. (A texture
> instruction returning an fp16 value can happen on some hardware).
> 
> My point is that if we do keep those hunks, they need to be
> squashed together with a tgsi_to_nir patch and corresponding
> back-end fixes.
> 

I have sent a v2 of the patch series without them. We can do these
hunks in one patch if needed in the future.

Thanks for the review!

Sam

>> And do the corresponding modifications to the driver.
>> 
>> Thanks!
>> 
>> Sam
>> 
>> [0]
>> 
> https://github.com/Igalia/mesa/commit/16c477ab84227d8bb9ffcba07091aef9a5e9f61e
>>
>>>>
> 
v.f32[0] = v.f32[1] = v.f32[2] = v.f32[3] = 1.0;
>>>>> else v.u32[0] = v.u32[1] = v.u32[2] = v.u32[3] = 1; -- 
>>>>> 2.5.0
>>>>> 
>>>>> _______________________________________________ mesa-dev
>>>>> mailing list mesa-dev at lists.freedesktop.org 
>>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>> 
>>>> 
>>>> 
>>> 
>