code review 108140045: go.image/vp8: remove the filter look-up tables.

go.image/vp8: remove the filter look-up tables.

The benchmarks are a little slower, but the code is shorter.

benchmark                          old ns/op     new ns/op     delta
BenchmarkDecodeVP8SimpleFilter     621950        644184        +3.57%
BenchmarkDecodeVP8NormalFilter     4045745       4269431       +5.53%

[nigeltao, 2014-06-26 03:43:05]

Hello r@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.image

[nigeltao, 2014-06-26 03:43:59]

The code is shorter, but the benchmarks are 4% slower. Personally, I'm not in
favor of this CL, but for discussion's sake, here are the numbers. WDYT?

[r, 2014-06-26 04:25:56]

Is the function in-lined?

On Wed, Jun 25, 2014 at 8:43 PM,  <nigeltao@golang.org> wrote:
> The code is shorter, but the benchmarks are 4% slower. Personally, I'm
> not in favor of this CL, but for discussion's sake, here are the
> numbers. WDYT?
>
> https://codereview.appspot.com/108140045/

[nigeltao, 2014-06-26 04:41:46]

On Thu, Jun 26, 2014 at 2:25 PM, Rob Pike <r@golang.org> wrote:
> Is the function in-lined?

Yes, it's being inlined. For example, given:

func clamp127(x int) int {
    if x < -128 {
        return -128
    }
    if x > 127 {
        return 127
    }
    return x
}

The code gen for:

a := 3*(q0-p0) + clamp127(p1-q1)

is:

        0x0127 00295 (filter.go:17)     MOVQ    R13,BP
        0x012a 00298 (filter.go:17)     SUBQ    R12,BP
        0x012d 00301 (filter.go:17)     CMPQ    BP,$-128
        0x0131 00305 (filter.go:17)     JGE     ,566
        0x0137 00311 (filter.go:17)     MOVQ    $-128,SI
        0x013e 00318 (filter.go:17)     MOVQ    DI,BP
        0x0141 00321 (filter.go:17)     SUBQ    R9,BP
        0x0144 00324 (filter.go:17)     IMULQ   $3,BP
        0x0148 00328 (filter.go:17)     ADDQ    SI,BP
        0x014b 00331 (filter.go:17)     MOVQ    BP,SI

and further down:

        0x0236 00566 (filter.go:17)     CMPQ    BP,$127
        0x023a 00570 (filter.go:17)     JLE     ,584
        0x023c 00572 (filter.go:17)     MOVQ    $127,SI
        0x0243 00579 (filter.go:17)     JMP     ,318
        0x0248 00584 (filter.go:17)     MOVQ    BP,SI
        0x024b 00587 (filter.go:17)     JMP     ,318

The compiler could possibly be more sophisticated in which branch is
the straight line (currently the "return -128") and which branch is
the jump-and-jump-back, but I'd expect that that's a lot of
sophistication.

[minux, 2014-06-26 05:02:11]

I'm in favor this CL. There really is no need to use lut for functions as
simple as abs and clamp.
I'd rather file an issue for the compiler.

[ruiu, 2014-06-26 06:24:55]

How about bit manipulation hacks? I don't know a good way to clamp a
number, but this branch-free abs might be faster than the naive one.

const intSize = 32 << uint(^uint(0)>>63)

func abs(x int) int {
        m := x >> (intSize - 1)
        return (x ^ m) - m
}


On Wed, Jun 25, 2014 at 10:01 PM, minux <minux@golang.org> wrote:

> I'm in favor this CL. There really is no need to use lut for functions as
> simple as abs and clamp.
> I'd rather file an issue for the compiler.
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[r, 2014-06-26 18:18:53]

LGTM

I like the cleanup. It's crazy to have such a big table just to clamp a value.
We can fine-tune the performance separately.

[nigeltao, 2014-06-26 23:15:43]

*** Submitted as
https://code.google.com/p/go/source/detail?r=9e772e41e630&repo=image ***

go.image/vp8: remove the filter look-up tables.

The benchmarks are a little slower, but the code is shorter.

benchmark                          old ns/op     new ns/op     delta
BenchmarkDecodeVP8SimpleFilter     621950        644184        +3.57%
BenchmarkDecodeVP8NormalFilter     4045745       4269431       +5.53%

LGTM=r
R=r, minux, ruiu
CC=golang-codereviews
https://codereview.appspot.com/108140045