On Tuesday, October 18, 2016 8:28:02 AM PDT Jason Ekstrand wrote: > On Tue, Oct 18, 2016 at 8:14 AM, Jason Ekstrand <ja...@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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
