Re: [PATCH v4 09/12] [media] vivid: Local optimization

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Ricardo,

Am Montag, den 18.07.2016, 15:21 +0200 schrieb Ricardo Ribalda Delgado:
> Hi Philipp
> 
> On Mon, Jul 18, 2016 at 3:13 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Since the constant expressions are evaluated at compile time, you are
> > not actually removing shifts. The code generated for precalculate_color
> > by gcc 5.4 even grows by one asr instruction with this patch.
> >
> 
> I dont think that I follow you completely here. The original code was

Sorry, I forgot to mention I compiled both versions for ARMv7-A, saw
that object size increased, had a look the diff between objdump -d
outputs and noticed an additional shift instruction. I have not checked
this for x86_64.

> if (a)
>    y= clamp(y, 16<<4, 235<<4)
> 
> y = clamp(y>>4, 1, 254)
>
> And now is
> 
> if (a)
>    y= clamp(y >>4, 16, 235)
> else
>     y = clamp(y, 1, 254)
     y = clamp(y >>4, 1, 254)

> On the previous case, when a was true there was 2 clamp operations.
> Now it is only one.

Yes. And now there's two shift operations (overall, still just one in
each conditional path).

It seems in my case the compiler was not clever enough to move all the
right shifts out of the conditional paths, so I ended up with one more
than before. You are right that in the limited range path the second
clamps are now avoided though. Basically, feel free to disregard my
comment.

I had the best looking result with this variant, btw:

	y >>= 4;
	cb >>= 4;
	cr >>= 4;
	if (tpg->real_quantization == V4L2_QUANTIZATION_LIM_RANGE) {
		y = clamp(y, 16, 235);
		cb = clamp(cb, 16, 240);
		cr = clamp(cr, 16, 240);
	} else {
		y = clamp(y, 1, 254);
		cb = clamp(cb, 1, 254);
		cr = clamp(cr, 1, 254);
	}

regards
Philipp