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: high inline cost of encoding/binary and math.Float32bits #42788

Closed
egonelbre opened this issue Nov 23, 2020 · 8 comments
Closed

cmd/compile: high inline cost of encoding/binary and math.Float32bits #42788

egonelbre opened this issue Nov 23, 2020 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@egonelbre
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
1.15.5

Does this issue reproduce with the latest release?

Yes, also happens on tip.

What did you do?

The inline cost calculation for these seem inconsistent https://go.godbolt.org/z/YMKWjc

package ex

import (
	"math"
	"encoding/binary"
)

type Point struct {
	X, Y float32
}

type Quad struct {
	From, Ctrl, To Point
}

func EncodeQuad(d [24]byte, q Quad) {
	binary.LittleEndian.PutUint32(d[0:], math.Float32bits(q.From.X))
	binary.LittleEndian.PutUint32(d[4:], math.Float32bits(q.From.Y))
	binary.LittleEndian.PutUint32(d[8:], math.Float32bits(q.Ctrl.X))
	binary.LittleEndian.PutUint32(d[12:], math.Float32bits(q.Ctrl.Y))
	binary.LittleEndian.PutUint32(d[16:], math.Float32bits(q.To.X))
	binary.LittleEndian.PutUint32(d[20:], math.Float32bits(q.To.Y))
}

func EncodeQuad2(d [6]float32, q Quad) {
	d[0] = q.From.X
	d[1] = q.From.Y
	d[2] = q.Ctrl.X
	d[3] = q.Ctrl.Y
	d[4] = q.To.X
	d[5] = q.To.Y
}

func EncodeQuad3(d [6]uint32, q Quad) {
	d[0] = math.Float32bits(q.From.X)
	d[1] = math.Float32bits(q.From.Y)
	d[2] = math.Float32bits(q.Ctrl.X)
	d[3] = math.Float32bits(q.Ctrl.Y)
	d[4] = math.Float32bits(q.To.X)
	d[5] = math.Float32bits(q.To.Y)
}

The inline cost for those:

$ go tool compile -m -m ex.go | grep cost
ex.go:16:6: cannot inline EncodeQuad: function too complex: cost 318 exceeds budget 80
ex.go:25:6: can inline EncodeQuad2 with cost 42 as: func([6]float32, Quad) { d[0] = q.From.X; d[1] = q.From.Y; d[2] = q.Ctrl.X; d[3] = q.Ctrl.Y; d[4] = q.To.X; d[5] = q.To.Y }
ex.go:34:6: cannot inline EncodeQuad3: function too complex: cost 90 exceeds budget 80

There seems to be a really large difference between EncodeQuad2, EncodedQuad3 and EncodeQuad. I would expect all of them to be inlinable and similar in cost.
@cespare
Copy link
Contributor

cespare commented Nov 23, 2020

As far as the Float32bits part goes, it could be similar to #42739.

@randall77
Copy link
Contributor

The big difference in EncodeQuad is that PutUint32 has to handle unaligned writes, whose expensiveness depends on the system. For architectures with unaligned writes it is cheap, but for those without unaligned writes it is still 24 byte writes. The other EncodeQuads can assume aligned writes.
At inline cost calculation time, we have to assume the worst for PutUint32 because it all depends on whether the unaligned write combining optimization happens.

BTW, EncodeQuad* doesn't do anything. It encodes into an argument that is thrown away upon return. You want to use a slice or pointer to an array, not a bare array.

@egonelbre
Copy link
Contributor Author

BTW, EncodeQuad* doesn't do anything. It encodes into an argument that is thrown away upon return. You want to use a slice or pointer to an array, not a bare array.

Thanks, I'm aware. The code is a simplification of #28941 (comment) to ensure BCE works.

@cagedmantis cagedmantis added this to the Backlog milestone Dec 2, 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 2, 2020
@egonelbre egonelbre mentioned this issue Dec 3, 2020
Closed
@egonelbre
Copy link
Contributor Author

egonelbre commented Jan 2, 2021 •
edited

I was thinking about the conversion overhead in math.Float32bits. Maybe (*X)(unsafe.Pointer(x)) should be considered as zero cost?

I currently added a new case in inl.go as:

	case OCONVNOP:
		if n.Type.IsPtr() && n.Left.Op == OCONVNOP && n.Left.Type.IsUnsafePtr() {
			// (*X)(unsafe.Pointer(x)) doesn't cost anything.
			return v.visit(n.Left.Left)
		}

I'm not entirely sure, whether the if handles all the conditions properly, however it did cut 2 off the cost of math.Float32bits.

F:\Temp\comp>go1.15.6 tool compile -m -m comp.go
comp.go:5:6: can inline Float32bits with cost 6 as: func(float32) uint32 { return *(*uint32)(unsafe.Pointer(&f)) }
comp.go:6:6: can inline Float32_return with cost 2 as: func(float32) float32 { return f }
comp.go:7:6: can inline Float32_deref with cost 4 as: func(float32) float32 { return *(&f) }

F:\Temp\comp>go tool compile -m -m comp.go
comp.go:5:6: can inline Float32bits with cost 4 as: func(float32) uint32 { return *(*uint32)(unsafe.Pointer(&f)) }
comp.go:6:6: can inline Float32_return with cost 2 as: func(float32) float32 { return f }
comp.go:7:6: can inline Float32_deref with cost 4 as: func(float32) float32 { return *(&f) }

It didn't make it equivalent to just returning, however it seems like a worthwhile improvement nevertheless.

@egonelbre
Copy link
Contributor Author

egonelbre commented Jan 2, 2021 •
edited

I guess the whole *(*X)(unsafe.Pointer(&x)) would be low-cost as long as the type x fits into a register.

This is more hacky case than the previous:

	case ODEREF:
		if n.Left.Op == OCONVNOP && n.Left.Type.IsPtr() && // (*X)(
			n.Left.Left.Op == OCONVNOP && n.Left.Left.Type.IsUnsafePtr() && // unsafe.Pointer(
			n.Left.Left.Left.Op == OADDR && // &
			n.Left.Left.Left.Left.Type != nil &&
			n.Left.Left.Left.Left.Type.Width <= 4 { // TODO: use register size
			// *(*X)(unsafe.Pointer(&x)) doesn't cost anything.
			return v.visit(n.Left.Left.Left.Left)
		}

@randall77
Copy link
Contributor

Perhaps all OCONVNOPs should be zero cost.

@egonelbre
Copy link
Contributor Author

Yeah, that is simpler. I wasn't sure whether string to byte (or any other similar) conversions are represented by OCONVNOP. After reading the documentation, they obviously aren't.

@gopherbot
Copy link

Change https://golang.org/cl/281232 mentions this issue: cmd/compile: reduce inline cost of OCONVOP

@jsternberg jsternberg mentioned this issue Jan 14, 2021
Closed
@golang golang locked and limited conversation to collaborators Feb 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

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