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

[Jason Ekstrand]

On Thu, Mar 17, 2016 at 5:15 AM, Iago Toral <itoral at igalia.com> wrote:

> 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.
>

I did a quick skim through, and it looks good.  Thanks for all your hard
work!
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160317/bfabf054/attachment.html>