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

cmd/compile: loop condition loop-invariant expression not lifted out of the loop #47919

Closed
prattmic opened this issue Aug 23, 2021 · 1 comment
Closed
Milestone

Comments

@prattmic
Copy link
Member

@prattmic prattmic commented Aug 23, 2021

package main

//go:noinline
func nop() {
}

//go:noinline
func loop(n, d int) {
	for i := 0; i < n*d; i++ {
		nop()
	}
}

func main() {
	loop(32, 16)
}

https://play.golang.org/p/RxXnFIJqtcj

loop compiles to:

00000 (29) TEXT "".loop(SB), ABIInternal
00001 (29) FUNCDATA $0, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
00002 (29) FUNCDATA $1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
00003 (29) FUNCDATA $5, "".loop.arginfo1(SB)
00004 (29) MOVQ AX, "".n(SP)
00005 (29) MOVQ BX, "".d+8(SP)
00006 (29) XORL CX, CX
00007 (30) JMP 15
00008 (30) MOVQ CX, "".i-8(SP)
00009 (+31) PCDATA $1, $0
00010 (+31) CALL "".nop(SB)
00011 (+30) MOVQ "".i-8(SP), AX
00012 (30) LEAQ 1(AX), CX
00013 (30) MOVQ "".n(SP), AX
00014 (30) MOVQ "".d+8(SP), BX
00015 (+30) IMULQ BX, AX
00016 (30) CMPQ CX, AX
00017 (30) JLT 8
00018 (33) PCDATA $1, $-1
00019 (33) RET

PCs 13-15 (IMULQ and associated loads) could be hoisted out of the loop to avoid re-computing the product on every iteration.


The original motivation of this issue is from an application I was profiling with a hot path something like:

func process(s []float64, d int) {
  for i := 0; i < len(s)/d; i++ {
    ... = asmVectorOps(s[i*d:(i+1)*d])
  }
}

The custom assembly used most of the time, but 5% of cycles still went to the division of len(s)/d on every iteration. I thought this was an optimization breaking down because it thought len(s) might change, but it turns out not to depend on slices at all.

cc @randall77 @dr2chase @mdempsky

Potentially related to or duplicate of #15808.

@prattmic prattmic added this to the Backlog milestone Aug 23, 2021
@prattmic
Copy link
Member Author

@prattmic prattmic commented Aug 23, 2021

After reading #15808 more closely, I'd say this is a duplicate.

Loading

@prattmic prattmic closed this Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant