On Thu, Mar 17, 2016 at 5:15 AM, Iago Toral <ito...@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 <ito...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
