[Mesa-dev] [PATCH 07/25] mesa/i965: eliminate gl_tess_ctrl_program and use new shared shader_info

[Kenneth Graunke]

On Tuesday, October 18, 2016 8:28:02 AM PDT Jason Ekstrand wrote:
> On Tue, Oct 18, 2016 at 8:14 AM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> 
> > I want to make a few comments on how this series is structured.  This is
> > not the way I would have done it and I think the way you structured it
> > makes it substantially less rebasable than it could be and a bit harder to
> > review.  The way *I* would have done this would be something like the
> > following:
> >
> >  1) Move shader_info to common code (patches 1-2)
> >  2) Add a shader_info pointer to gl_program (patch 6), break the fill
> > shader_info stuff from glsl_to_nir into its own function, and call it from
> > somewhere such that it always gets filled out.
> >  3) Add new fields to shader_info *and* make sure they get filled out from
> > other GLSL information
> >  4) Convert i965 over to the new shader_info
> >  5) Convert gallium over to the new shader_info
> >  6) Make GLSL fill out shader_info directly and nuke the old shader
> > metadata.
> >  7) Delete the shader_info fill-out function.
> >
> > Something along these lines would go a long way towards avoiding the "mega
> > patch" problem where each patch touches 4 or 5 different components.  It
> > also makes it clearer to review because you don't add fields and then the
> > reviewer goes "Wait, where does this get set?  Oh, in another patch".  I'm
> > not necessarily saying that you have to go back and change your patches.
> > It's more a suggestion for if you end up doing a v3 or another refactor
> > along these lines in the future.
> >
> 
> On the review side, splitting out as I described above would make it much
> easier to review since it would be more-or-less one type of refactor per
> patch.  In this patch, we have several different kinds of refactors:
> 
>  1) Move consumers over to reading shader_info
>  2) Remove gl_tess_ctrl_program and related refactors
>  3) Move producer over to writing shader_info
> 
> Normally, when reviewing, I would just skim (2) and give (1) a (3) more
> effort.  Having them mixed together means I have to pay constant attention
> to what's going on.  Also, having (2) mixed in makes it harder to verify
> (3) because there's a lot of code motion only some of which matters.

I agree with Jason.  This could be structured a lot more cleanly, and it
would make it much easier to review.

For example, patches 3-5 add a bunch of new structure fields.  But they
aren't populated by anything.  The CS local_size_variable field finally
gets populated in patch 12 (a whole 7 patches later!)...and by the end
of the series...I don't see a single consumer of that field.

So, the field is useless.  But I had to use 'git log -p' on a branch and
search through your entire series to determine that.  There's far too
much context to keep in my head while reading, and it means I have to
abandon my usual read-emails-mostly-in-order review process.

I actually added TES shader info a little while back (but hadn't sent
them out yet as Vulkan tessellation isn't quite ready yet).  Here's what
my patches looked like:

1) Convert spacing from GLenums to a TESS_SPACING_* enum
https://cgit.freedesktop.org/~kwg/mesa/commit/?h=vktess&id=8b49a8485dd37eb405efcaaecd55244a8f63f213
   (simple cleanup I did across the whole codebase)

2) Introduce nir_shader_info fields and populate them in glsl_to_nir and
   spirv_to_nir.
https://cgit.freedesktop.org/~kwg/mesa/commit/?h=vktess&id=3142efa913965324ad21c3cefc792ab83e1a1390
   (fields are at least populated in all frontends, but may be useless)

3) Convert i965 over to use nir_shader_info for fields
https://cgit.freedesktop.org/~kwg/mesa/commit/?h=vktess&id=a518388acc7a6db88c7e21829e7a15b15b9304ad
   (now the fields are used.  admittedly I did some bonus code motion in
    this patch...if I'm being pedantic, I should have made that a fourth
    patch to make the prog_data fields be populated in Vulkan paths)

The first patch stands alone, and patches 2-3 stand together.  All are
very small.  You need no additional context to answer questions, and can
say "those look good" and move on rather quickly.

With that in mind, I'd like to ask you to please try and rework this
series along the lines that Jason suggested.  I know it's a bunch of
work, but being disciplined in how we organize our code is a really
useful skill that pays off in the long run.  When reviewers can look
at your code and quickly give a thumbs up, you get to land your patches
a lot more quickly, and that extra effort ultimately saves you (and
others) a whole lot of time.

Sorry, Tim :(

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161018/28502f35/attachment.sig>