Removing unnecessary temporary variable(sum) in fragmentshader

Removing unnecessary temporary variable(sum) in fragmentshader

[bsalomon, 2012-06-13 12:37:39]

Adding Tom who wrote this code.  I know that some shader compilers are pretty
light on optimizations, especially on mobile platforms. So I'm not opposed to
the change but I'm curious to know if you observed a speedup anywhere with this
change.

https://codereview.appspot.com/6302070/diff/1/src/gpu/effects/GrConvolutionEf...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

https://codereview.appspot.com/6302070/diff/1/src/gpu/effects/GrConvolutionEf...
src/gpu/effects/GrConvolutionEffect.cpp:105:
code->appendf(state->fModulate.c_str());
I'm not sure I get this line. Don't we need something like:

if (state->fModulate.length()) {
    code->appendf("\t\t%s = %s%s\n", outputColor, outputColor,
state->fModulate.c_str());
}

Maybe I'm not correctly remembering the interpretation of  state->fModulate.
}

[TomH, 2012-06-13 12:52:43]

https://codereview.appspot.com/6302070/diff/1/src/gpu/effects/GrConvolutionEf...
File src/gpu/effects/GrConvolutionEffect.cpp (right):

https://codereview.appspot.com/6302070/diff/1/src/gpu/effects/GrConvolutionEf...
src/gpu/effects/GrConvolutionEffect.cpp:105:
code->appendf(state->fModulate.c_str());
Brian's right; the patch as submitted fails
  out/Debug/tests --match GLPrograms
Using Brian's snippet (but with fModulate.size(), not length()) passes tests,
gm, and bench.

With those changes LGTM, but I agree with Brian that it'd be nice to know that
we're getting speedup somewhere before taking this patch. Given the failure of
unit tests, were you able to run this at all before submitting it?

[kondapallykalyan, 2012-06-13 17:56:17]

On 2012/06/13 12:52:43, TomH wrote:
>
https://codereview.appspot.com/6302070/diff/1/src/gpu/effects/GrConvolutionEf...
> File src/gpu/effects/GrConvolutionEffect.cpp (right):
> 
>
https://codereview.appspot.com/6302070/diff/1/src/gpu/effects/GrConvolutionEf...
> src/gpu/effects/GrConvolutionEffect.cpp:105:
> code->appendf(state->fModulate.c_str());
> Brian's right; the patch as submitted fails
>   out/Debug/tests --match GLPrograms
> Using Brian's snippet (but with fModulate.size(), not length()) passes tests,
> gm, and bench.
> 
> With those changes LGTM, but I agree with Brian that it'd be nice to know that
> we're getting speedup somewhere before taking this patch. Given the failure of
> unit tests, were you able to run this at all before submitting it?

Thanks for your comments and fix.I had the proposed fix in my second patch and
was certain that it got uploaded to the review page.It should be there at least
now, I am sorry about this.

I ran benchmark tests on my linux laptop(uses intel sandybridge) and saw a
speedup of 0.5 to 1%. Unfortunately, I dont have any other devices to benchmark
this. Benchmark Results are as follows:

Used command: out/Release/Bench -repeat 1000 -config GPU -match blur

skia bench: alpha=0xFF antialias=1 filter=0 deferred=0 rotate=0 scale=0 clip=0
dither=default strokeWidth=none scalar=float system=UNIX

Results: 
Without Patch:
[640 480] blur_0_none   GPU: cmsecs =   0.56 gmsecs =   0.55
[640 480] blur_1.50_inner   GPU: cmsecs =  15.91 gmsecs =  16.03
[640 480] blur_1.50_outer   GPU: cmsecs =  15.91 gmsecs =  16.03
[640 480] blur_1.50_solid   GPU: cmsecs =  15.92 gmsecs =  16.06
[640 480] blur_1.50_normal   GPU: cmsecs =  13.00 gmsecs =  13.12
[640 480] blur_10_inner   GPU: cmsecs =  20.52 gmsecs =  20.63
[640 480] blur_10_outer   GPU: cmsecs =  20.60 gmsecs =  20.72
[640 480] blur_10_solid   GPU: cmsecs =  20.65 gmsecs =  20.77
[640 480] blur_10_normal   GPU: cmsecs =  17.83 gmsecs =  17.94
[640 480] blur_2_inner   GPU: cmsecs =  15.97 gmsecs =  16.08
[640 480] blur_2_outer   GPU: cmsecs =  15.96 gmsecs =  16.10
[640 480] blur_2_solid   GPU: cmsecs =  16.00 gmsecs =  16.14
[640 480] blur_2_normal   GPU: cmsecs =  13.03 gmsecs =  13.15

With Patch:

[640 480]   blur_0_none   GPU: cmsecs =   0.56 gmsecs =   0.54
[640 480]   blur_1.50_inner   GPU: cmsecs =  15.72 gmsecs =  15.85
[640 480]   blur_1.50_outer   GPU: cmsecs =  15.80 gmsecs =  16.01
[640 480]   blur_1.50_solid   GPU: cmsecs =  15.68 gmsecs =  15.75
[640 480]   blur_1.50_normal   GPU: cmsecs =  12.80 gmsecs =  12.85
[640 480]   blur_10_inner   GPU: cmsecs =  20.41 gmsecs =  20.53
[640 480]   blur_10_outer   GPU: cmsecs =  20.29 gmsecs =  20.36
[640 480]   blur_10_solid   GPU: cmsecs =  20.35 gmsecs =  20.43
[640 480]   blur_10_normal   GPU: cmsecs =  17.49 gmsecs =  17.55
[640 480]   blur_2_inner   GPU: cmsecs =  15.76 gmsecs =  15.85
[640 480]   blur_2_outer   GPU: cmsecs =  15.70 gmsecs =  15.78
[640 480]   blur_2_solid   GPU: cmsecs =  15.77 gmsecs =  15.86
[640 480]   blur_2_normal   GPU: cmsecs =  12.88 gmsecs =  12.94

[TomH, 2012-06-13 18:13:04]

Even with -repeat 1000, the GPU tests can see 1% noise, but we'll go ahead and
take the change. Thanks for contributing!

Committed with r4250.