Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/compile: improve code generation for math.Abs #13095

Closed
bradfitz opened this issue Oct 29, 2015 · 10 comments
Closed

cmd/compile: improve code generation for math.Abs #13095

bradfitz opened this issue Oct 29, 2015 · 10 comments
Labels
FrozenDueToAge Performance
Milestone
Unplanned

Comments

@bradfitz
Copy link
Contributor

From the thread,

"Pure Go math.Abs outperforms assembly version"
https://groups.google.com/forum/#!topic/golang-dev/nP5mWvwAXZo

It was noted that the code generation for

func Abs(x float64) float64 {
        return Float64frombits(Float64bits(x) &^ (1 << 63))
}

is not ideal:

@randall77 wrote:

Current assembly:

    MOVQ   $(1<<63), BX
    MOVQ   BX, X0 // movsd $(-0.0), x0
    MOVSD  x+0(FP), X1
    ANDNPD X1, X0
    MOVSD  X0, ret+8(FP)
    RET

Generated by the compiler from Russ' example:

    0x0000 00000 (abs.go:5) SUBQ    $16, SP
    0x0004 00004 (abs.go:5) XORPS   X0, X0
    0x0007 00007 (abs.go:6) MOVSD   "".x+24(FP), X0
    0x000d 00013 (abs.go:6) MOVSD   X0, math.f·2(SP)
    0x0012 00018 (abs.go:6) LEAQ    math.f·2(SP), BX
    0x0016 00022 (abs.go:6) MOVQ    (BX), BX
    0x0019 00025 (abs.go:6) MOVQ    $9223372036854775807, BP
    0x0023 00035 (abs.go:6) ANDQ    BP, BX
    0x0026 00038 (abs.go:6) MOVQ    BX, math.b·2+8(SP)
    0x002b 00043 (abs.go:6) XORPS   X0, X0
    0x002e 00046 (abs.go:6) LEAQ    math.b·2+8(SP), BX
    0x0033 00051 (abs.go:6) MOVSD   (BX), X0
    0x0037 00055 (abs.go:6) MOVSD   X0, "".~r1+32(FP)
    0x003d 00061 (abs.go:6) ADDQ    $16, SP
    0x0041 00065 (abs.go:6) RET

...

SSA:
    0x0000 00000 (abs.go:5) SUBQ    $16, SP
    0x0004 00004 (abs.go:5) MOVQ    $0, "".~r1+32(FP)
    0x000d 00013 (abs.go:5) MOVSD   "".x+16(SP), X0
    0x0013 00019 (abs.go:6) MOVSD   X0, math.f·2(SP)
    0x0018 00024 (abs.go:6) MOVQ    math.f·2(SP), AX
    0x001c 00028 (abs.go:6) MOVQ    $9223372036854775807, CX
    0x0026 00038 (abs.go:6) ANDQ    CX, AX
    0x0029 00041 (abs.go:6) MOVQ    AX, math.b·2+8(SP)
    0x002e 00046 (abs.go:6) MOVSD   math.b·2+8(SP), X0
    0x0034 00052 (abs.go:6) MOVSD   X0, "".~r1+32(FP)
    0x003a 00058 (abs.go:6) ADDQ    $16, SP
    0x003e 00062 (abs.go:6) RET

better hand-written code:
    MOVQ   x+0(FP), AX
    SHLQ   $1, AX
    SHRQ   $1, AX
    MOVQ   AX, ret+8(FP)
    RET

This is bug about improving it.
@bradfitz bradfitz added this to the Unplanned milestone Oct 29, 2015
@gopherbot
Copy link

CL https://golang.org/cl/16444 mentions this issue.

bradfitz added a commit that referenced this issue Oct 29, 2015
@bradfitz
math: replace assembly implementations of Abs with pure Go version
6f8a665 
The compiler can do a fine job, and can also inline it.

From Jeremy Jackins's observation and rsc's recommendation in thread:

"Pure Go math.Abs outperforms assembly version"
https://groups.google.com/forum/#!topic/golang-dev/nP5mWvwAXZo

Updates #13095

Change-Id: I3066f8eaa327bb403173b29791cc8661d7c0532c
Reviewed-on: https://go-review.googlesource.com/16444
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@randall77
Copy link
Contributor

Mostly the problem is about getting rid of the stores.

Step 1 would be an SSA rule that recognizes that a store followed by a load from the same address can short-circuit the store and use the store's argument directly instead of the load.

Step 2 would be to recognize that stores to auto variables that are never read again can be dropped.

Step 3 is to replace the and with two shifts.

@gopherbot
Copy link

CL https://golang.org/cl/19482 mentions this issue.

@minux
Copy link
Member

minux commented Feb 17, 2016

Fixed by https://golang.org/cl/19485.

@minux minux closed this as completed Feb 17, 2016
@minux
Copy link
Member

minux commented Feb 17, 2016

Turns out CL 19485 is not enough, see discussion on the CL.

@minux minux reopened this Feb 17, 2016
@bradfitz
Copy link
Contributor Author

@randall77, not sure whether you're continuing your intrinsification crusade, but maybe the popular funcs at least in math are candidates.

(Reminded after seeing @mundaym's https://go-review.googlesource.com/27827 and the TODO in https://golang.org/src/math/abs.go?s=278:305#L2:

    12  func Abs(x float64) float64 {
    13      // TODO: once golang.org/issue/13095 is fixed, change this to:
    14      // return Float64frombits(Float64bits(x) &^ (1 << 63))
    15      // But for now, this generates better code and can also be inlined:
    16      if x < 0 {
    17          return -x
    18      }
    19      if x == 0 {
    20          return 0 // return correctly abs(-0)
    21      }
    22      return x
    23  }

@randall77
Copy link
Contributor

I think I will continue intrinsification to sync/atomic, but probably stop there. It feels wrong to intrinsify something that's easily writeable in Go. And I still have hope for doing this more generically.

@josharian josharian mentioned this issue Sep 12, 2016
Closed
This was referenced Sep 23, 2016
Open
Open
@randall77 randall77 mentioned this issue Mar 29, 2017
Closed
@randall77 randall77 mentioned this issue Aug 14, 2017
Closed
@gopherbot
Copy link

Change https://golang.org/cl/58732 mentions this issue: cmd/compile,math: improve code generation for math.Abs

@mundaym mundaym mentioned this issue Sep 8, 2017
Closed
@gopherbot
Copy link

Change https://golang.org/cl/63050 mentions this issue: math: Use Abs rather than if x < 0 { x = -x}

@gopherbot
Copy link

Change https://golang.org/cl/63190 mentions this issue: math: Use Abs rather than if x < 0 { x = -x }

@golang golang locked and limited conversation to collaborators Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@bradfitz @minux @randall77 @gopherbot