On Fri, Jan 31, 2014 at 1:22 PM, Carl Worth <cwo...@cworth.org> wrote: > Matt Turner <matts...@gmail.com> writes: >> --- >> src/glsl/opt_vectorize.cpp | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) > > Hi Matt, > > I'm missing the rest of your commit message besides the one-line > summary. > > I think the discipline of always typing _something_ there is valuable. I > like to call the one-line summary the "what" of the commit message, and > the following paragraph the "why". > > And it's the "why" that's particularly important during review. > > In this particular case, I'm not very familiar with the code being > modified, (I'm trying to do more active review of mesa patches as a way > of getting more familiar with the code). A bit more of the why would > help me in my review here. What's the value of the change here? > Correctness or optimization? Do you have example code sequences that > would not be operated-on by the previous code, but that will be by the > new code? > > You don't need to answer all of those questions in the commit > message. But I bet if you wrote a sentence or two, I'd have pretty good > guidance for at least some of those answers. > > Thanks for helping out a Mesa newbie here,
Hi Carl, Thanks for the comments. The context of this patch is Aras' email titled "glsl: vectorize pass probably needs to change types of scalar constants as well?" Essentially, my vectorization pass needs to expand scalar operands that are reused in multiple operations into vectors. E.g., when combining a.x = pow(b.x, 2.5); a.y = pow(b.y, 2.5); into a.xy = pow(b.xy, vec2(2.5)); we need to expand 2.5 into vec2(2.5), which we do with a swizzle. My original code only expanded scalar variable dereferences, but Aras pointed out that other rvalues should get the same treatment. That's what this patch does. I should have explained that in the commit message. I've already pushed the patch after Aras said it looked good to him, but I wasn't able to coax a Reviewed-by out of him. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
