[Mesa-dev] Split gl_shader in two and clean-ups

[Timothy Arceri]

On Thu, 2016-06-30 at 08:15 +0200, Iago Toral wrote:
> On Wed, 2016-06-29 at 23:12 +1000, Timothy Arceri wrote:
> > On Wed, 2016-06-29 at 14:42 +0200, Iago Toral wrote:
> > > On Wed, 2016-06-29 at 14:40 +0200, Iago Toral wrote:
> > > > On Tue, 2016-06-28 at 16:30 +0200, Iago Toral wrote:
> > > > > On Tue, 2016-06-28 at 11:52 +1000, Timothy Arceri wrote:
> > > > > > There are two distinctly different uses of this struct. The
> > > > > > first
> > > > > > is to store GL shader objects. The second is to store
> > > > > > information
> > > > > > about a shader stage thats been linked.
> > > > > > 
> > > > > > The only place the new structs overlap is the shader layout
> > > > > > fields and
> > > > > > I intend to split that out into a third struct once this
> > > > > > series
> > > > > > lands.
> > > > > > 
> > > > > > Having two well defined structs helps code readability and
> > > > > > allows the removal
> > > > > > of some unreachable code paths that were the result of
> > > > > > confusion between
> > > > > > the two uses.
> > > > > 
> > > > > I think it is a good idea, thanks!
> > > > > 
> > > > > I dropped a comment in patch 4, with that fixed patches 1-4
> > > > > are:
> > > > > Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> > > > > 
> > > > > I'll try to review the last 3 patches tomorrow.
> > > > 
> > > > Patches 5-6 are:
> > > 
> > > I meant 6-7 here...
> > > 
> > > > Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> > > > 
> > > > Patch 4 is huge so it is kind difficult to review it in detail,
> > > > it
> > > > looks
> > > 
> > > ... and patch 5 here, sorry for the mess.
> > > 
> > > > good to me in general but I dropped comments for few things
> > > > that
> > > > caught
> > > > my eye.
> > 
> > With those comments addressed do I have a r-b for 5 too? I'd settle
> > for
> > an acked-by  :)
> 
> Yeah, you can add my Ack with those fixed.
> 
> > 
> > > > 
> > > > BTW, I think you should test the series against CTS too if you
> > > > haven't
> > > > already and maybe we should test this on something other than
> > > > Intel
> > > > too
> > > > to be sure.
> > 
> > No regressions using Intels Jenkins setup :) I guess I could try it
> > on
> > the r600 driver if I can manage to setup a dev environment on my
> > old
> > desktop. Unless there is someone out there kind enough to give the
> > series a run for me?
> 
> I was hoping that you could ping someone to give this a run on some
> other hardware, maybe Marek? At the very least you should build as
> many
> drivers as you can, but I guess you already did that.
> 
> If you can't test this on non-Intel hardware, knowing that Intel is
> okay
> I guess it would be fine to push and have people try this once it
> lands
> in master.

I asked on IRC and llvmpipe was suggested. That *should* be good enough
as I don't think these structs are used past the glsl_to_tgsi
conversion.

Just finishing a run on master now to compare with but looks like
everything is ok.

Thanks again for the reviews.



> 
> Iago
> 
> > > > 
> > > > Iago
> > > 
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev