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

[Jason Ekstrand]

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.

> 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
> >>>
> >>
> >>
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160323/102b08c1/attachment.html>