<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Mar 17, 2016 at 5:15 AM, Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, 2016-03-16 at 10:54 -0700, Jason Ekstrand wrote: > > > On Wed, Mar 16, 2016 at 2:01 AM, Iago Toral <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> 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. </div></div>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. </blockquote><div> </div><div>I did a quick skim through, and it looks good. Thanks for all your hard work! </div><div>--Jason</div></div> </div></div>