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: generate better SSA code for shift-by-constant #12022

Closed
josharian opened this issue Aug 4, 2015 · 4 comments
Closed

cmd/compile: generate better SSA code for shift-by-constant #12022

josharian opened this issue Aug 4, 2015 · 4 comments
Labels
FrozenDueToAge
Milestone
Go1.6

Comments

@josharian
Copy link
Contributor 

package p

func f_ssa(x uint8) uint8 {
    return x >> 4
}

does not account for the fact that 4 is a constant, and instead does a shift+mask dance. Just a question of adding a bunch of new rewrite rules.

Currently it generates:

"".f_ssa t=1 size=64 value=0 args=0x10 locals=0x8
    0x0000 00000 (x.go:3)   TEXT    "".f_ssa(SB), $8-16
    0x0000 00000 (x.go:3)   SUBQ    $8, SP
    0x0004 00004 (x.go:3)   FUNCDATA    $0, "".gcargs·0(SB)
    0x0004 00004 (x.go:3)   FUNCDATA    $1, "".gclocals·1(SB)
    0x0004 00004 (x.go:3)   LEAQ    16(SP), AX
    0x0009 00009 (x.go:3)   MOVB    (AX), CL
    0x000b 00011 (x.go:3)   MOVB    CL, (SP)
    0x000e 00014 (x.go:4)   MOVQ    $4, CX
    0x0015 00021 (x.go:3)   MOVB    (SP), BL
    0x0018 00024 (x.go:4)   MOVB    BL, BPB
    0x001b 00027 (x.go:4)   SHRB    CL, BPB
    0x001e 00030 (x.go:4)   CMPQ    CX, $8
    0x0022 00034 (x.go:4)   SBBL    SI, SI
    0x0024 00036 (x.go:4)   MOVB    BPB, DIB
    0x0027 00039 (x.go:4)   ANDB    SIB, DIB
    0x002a 00042 (x.go:3)   LEAQ    24(SP), R8
    0x002f 00047 (x.go:3)   MOVB    $0, R9B
    0x0032 00050 (x.go:3)   MOVB    R9B, (R8)
    0x0035 00053 (x.go:4)   MOVB    DIB, (R8)
    0x0038 00056 (x.go:3)   ADDQ    $8, SP
    0x003c 00060 (x.go:3)   RET

It should be much closer to the old backend:

"".f_ssa t=1 size=16 value=0 args=0x10 locals=0x0
    0x0000 00000 (x.go:3)   TEXT    "".f_ssa(SB), $0-16
    0x0000 00000 (x.go:3)   FUNCDATA    $0, gclocals·23e8278e2b69a3a75fa59b23c49ed6ad(SB)
    0x0000 00000 (x.go:3)   FUNCDATA    $1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
    0x0000 00000 (x.go:4)   MOVBQZX "".x+8(FP), BX
    0x0005 00005 (x.go:4)   SHRB    $4, BL
    0x0008 00008 (x.go:4)   MOVB    BL, "".~r1+16(FP)
    0x000c 00012 (x.go:4)   RET

/cc @randall77 @brtzsnr
@brtzsnr
Copy link
Contributor

brtzsnr commented Aug 5, 2015

I saw the issue when working on: #10637. You can assign it to me, but I'll only have time next week.

@josharian
Copy link
Contributor Author

@brtzsnr no rush at all. I just wasn't going to fix it myself (at least not immediately) and didn't want to forget about it.

@gopherbot
Copy link

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

@rsc rsc added this to the Go1.6 milestone Aug 6, 2015
gopherbot pushed a commit that referenced this issue Aug 6, 2015
@randall77
[dev.ssa] cmd/compile/internal/ssa/gen: generate better code when rig…
d2150c8 
…ht-shifting with a constant.

The lowering rules were missing the non-64 bit case.

SBBLcarrymask can be folded to a int32 integer whose
type has a smaller bit size. Without the new AND rules
the following would be generated:

    v19 = MOVLconst <uint8> [-1] : SI
    v20 = ANDB <uint8> v18 v19 : DI

which is obviously a NOP.

Fixes #12022

Change-Id: I5f4209f78edc0f118e5b9b2908739f09cefebca4
Reviewed-on: https://go-review.googlesource.com/13301
Reviewed-by: Keith Randall <khr@golang.org>
@josharian
Copy link
Contributor Author

Apparently the fixes syntax only works for the master branch. This was fixed by CL 13301.

@golang golang locked and limited conversation to collaborators Aug 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.6
Development

No branches or pull requests
4 participants
@brtzsnr @josharian @rsc @gopherbot