Navigation Menu

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.

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: inlined err check elimination opportunity #42999

Open
egonelbre opened this issue Dec 4, 2020 · 2 comments
Open

cmd/compile: inlined err check elimination opportunity #42999

egonelbre opened this issue Dec 4, 2020 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@egonelbre
Copy link
Contributor

I realized that there is a if err != nil branch elimination possibility after inlined func.

With go tip 2c2980a, the following code (https://go.godbolt.org/z/sx5Gr9):

package ex

func fuse(v int) (int, error) {
	s, err := sqr(v)
	if err != nil {
		return -1, err
	}
	return s, nil
}

func sqr(v int) (int, error) {
	if v < 0 {
		return 0, errorString("xyz")
	}
	return v * v, nil
}

type errorString string

func (s errorString) Error() string { return string(s) }

This is compiled into:

        movq    "".v+8(SP), AX
        testq   AX, AX
        jge     fuse_pc65
        leaq    go.itab."".errorString,error(SB), AX
        xorl    CX, CX
        leaq    ""..stmp_0(SB), DX
fuse_pc26:
        testq   AX, AX // <--- here it should be possible to know that `AX` is not-nil
        jeq     fuse_pc51
        movq    $-1, "".~r1+16(SP)
        movq    AX, "".~r2+24(SP)
        movq    DX, "".~r2+32(SP)
        ret
fuse_pc51:
        movq    CX, "".~r1+16(SP)
        xorps   X0, X0
        movups  X0, "".~r2+24(SP)
        ret
fuse_pc65:
        imulq   AX, AX
        movq    AX, CX
        xorl    DX, DX
        xorl    AX, AX
        jmp     fuse_pc26

If I manually do the transform, the code ends up looking like:

func fuse(v int) (int, error) {
    if v < 0 {
        return -1, errorString("xyz")
    }
	return v*v, nil
}

Which, in assembly is:

        movq    "".v+8(SP), AX
        testq   AX, AX
        jge     fuse_pc65
        movq    $-1, "".~r1+16(SP)
        leaq    go.itab."".errorString,error(SB), AX
        movq    AX, "".~r2+24(SP)
        leaq    ""..stmp_0(SB), AX
        movq    AX, "".~r2+32(SP)
        ret
fuse_pc65:
        imulq   AX, AX
        movq    AX, "".~r1+16(SP)
        xorps   X0, X0
        movups  X0, "".~r2+24(SP)
        ret

I'm not sure for how many err != nil it would be possible, however, already for a few, it should improve things.

When creating the err inside the func, it's able to remove the branch:

func fuse(v int) (int, error) {
	var err error = errorString("xyz")
	if err != nil {
		return -1, err
	}
	return v * v, nil
}

Compiles into:

        movq    $-1, "".~r1+16(SP)
        leaq    go.itab."".errorString,error(SB), AX
        movq    AX, "".~r2+24(SP)
        leaq    ""..stmp_0(SB), AX
        movq    AX, "".~r2+32(SP)
        ret
@egonelbre
Copy link
Contributor Author

Could be related to #17862

@randall77
Copy link
Contributor

There's a recombination of control flow happening here that the compiler isn't currently able to disentangle.
Basically, the inlined version is doing this:

func fuse(v int) (int, error) {
	var s int
	var err error
	if v < 0 {
		s = -1
		err = errorString("xyz")
	} else {
		s = v*v
		err = nil
	}
	// recombination here
	if err != nil {
		return -1, err
	}
	return s, nil
}

This should certainly be optimizable. Try patching in https://go-review.googlesource.com/c/go/+/239457 , it may just be that optimization. But maybe not quite, because there's one pesky instruction in one of the branches that needs to be lifted first?

At the fuse pass, we have

b3: ← b1
Plain → b2 (+27)

b4: ← b1-
v16 (+29) = Mul64 <int> v7 v7 (s[int])
Plain → b2 (+30)

b2: ← b3 b4-
v32 (35) = Phi <int> v11 v16 (s[int])
v31 (32) = Phi <uintptr> v12 v20 (err.itab[uintptr])
v4 (32) = Phi <*uint8> v13 v19 (err.data[*uint8])
v18 (+32) = IsNonNil <bool> v31
If v18 → b6 b5 (32)

The Mul64 might prevent that CL from helping.
@erifan

@randall77 randall77 added this to the Unplanned milestone Dec 4, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 4, 2020
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@cagedmantis @egonelbre @bcmills @randall77 @gopherbot