[Mesa-dev] [PATCH] glsl: Expand non-expr & non-swizzle scalar rvalues in vectorizing.

[Matt Turner]

On Fri, Jan 31, 2014 at 1:22 PM, Carl Worth <cworth at cworth.org> wrote:
> Matt Turner <mattst88 at 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.