[Mesa-dev] [PATCH 13/14] nir: propagate bitsize information in nir_search

[Iago Toral]

On Wed, 2016-03-16 at 10:54 -0700, Jason Ekstrand wrote:
> 
> 
> On Wed, Mar 16, 2016 at 2:01 AM, Iago Toral <itoral at igalia.com> wrote:
>         On Wed, 2016-03-16 at 09:48 +0100, Samuel Iglesias Gonsálvez
>         wrote:
>         >
>         > On 15/03/16 08:41, Iago Toral wrote:
>         > > On Mon, 2016-03-14 at 17:33 -0700, Jason Ekstrand wrote:
>         > [...]
>         > >> +   nir_alu_type type; + union { -      uint32_t u; -
>         > >> int32_t i; -      float f; +      uint64_t u; +
>         int64_t i; +
>         > >> double d; } data; } nir_search_constant;
>         > >
>         > > Maybe we should rename these to u64, i64 as f64 like you
>         suggested
>         > > for a similar case in another patch?
>         > >
>         >
>         > We decided to do the following changes to this patch series
>         (among the
>         > others already agreed): rename these fields and add the
>         inline helper
>         > functions to get type size and base type from nir_alu_type
>         enum
>         > values. So the rest of the fp64 patches will have those
>         changes
>         > already and everything would be easier to maintain.
>         >
>         > One question for Jason: Should we squash all the patches of
>         this
>         > series in a single commit as originally proposed? I'd rather
>         have them
>         > separately so we understand what each patch does in case of
>         a git
>         > bisect, however I don't have a strong opinion against
>         squashing.
>         
>         At the very least we should squash the one for the
>         copy-propagation pass
>         with the new opcodes, otherwise we'd break the build.
>         
>         The main concern with squashing everything is that it is going
>         to be a
>         huge patch and it is going to be difficult to make sense of it
>         like that
>         for anyone who tracks something down to it, but otherwise we
>         are going
>         to break some things in between which is also not ideal...
> 
> 
> Right.  Let's squash the minimum.  I think we can probably keep copy
> propagation and nir_search mostly out of the squash.  Right now,
> nothing uses values of any size other than 32 so it should be safe to
> leave them separate for the most part.

Ok, we have addressed all the comments and pushed this.

For the record:

We had to squash copy propagation, since otherwise we would break the
build. We kept nir_search separate even though that means that we have a
small number if regressions for 3 commits in the series (we counted 4
regressions in shader.py). The reason is that otherwise we would have to
squash the changes to nir_search.c together with the changes to opcodes
and copy propagation, and we would end up with a huge patch again that
would be difficult to make sense of. I hope this is okay.

We also did the change to rename nir_constant_value's fields f->f32,
d->f64, u->u32, ul->u64, etc  This had to be done after the
copy-propagation changes, since the original implementation relied on
the naming convention for these fields to work. We kept this change
separate since it is just a large rename and squashing it into anything
else would just obscure things.

We also added the helper functions to obtain the base type and size of
nir types and used them throughout the series.

Iago