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

x/tools/internal/refactor/inline: allow conversions to be considered duplicable #67589

Open
vladvalkov opened this issue May 22, 2024 · 5 comments · May be fixed by golang/tools#494
Open

x/tools/internal/refactor/inline: allow conversions to be considered duplicable #67589

vladvalkov opened this issue May 22, 2024 · 5 comments · May be fixed by golang/tools#494
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@vladvalkov
Copy link

A subtask of #63352.

At the moment, inliner does not consider type conversions as duplicable operations.
For example, inlining a function f

func f(a int) {
    print(a, a)
}

func main() {
    var a int8 = 1
    f(int(a))
}

generates this code:

func main() {
    var a int8 = 1
    var s int = int(a)
    print(s, s)
}

Instead, inliner should consider some conversions (those that do not change semantics or involve additional allocations) to be duplicable:

func main() {
    var a int8 = 1
    print(int(s), int(s))
}

One way to implement this is by changing duplicable function to cover the type conversion cases, which are parsed as *ast.CallExpr with the Fun field being one of *ast.Ident, *ast.ArrayType, *ast.StructType, *ast.FuncType, *ast.InterfaceType, *ast.MapType, or *ast.ChanType.

As per spec, string([]byte) and []byte(string) conversions may involve additional allocations and should not be considered duplicable:

Specific rules apply to (non-constant) conversions between numeric types or to and from a string type. These conversions may change the representation of x and incur a run-time cost. All other conversions only change the type but not the representation of x.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label May 22, 2024
@gopherbot gopherbot added this to the Unreleased milestone May 22, 2024
@vladvalkov
Copy link
Author

@adonovan If I am not missing anything, I'd love to implement the change

@adonovan
Copy link
Member

I'd love to implement the change

Have at it!

I think the string-to-(rune/byte)-slice conversions are the only ones that observably allocate. Even duplicating string([]byte) won't break a program, though it may make it less efficient.

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 23, 2024
@vladvalkov
Copy link
Author

@adonovan
I did some tests, and apparently, the inliner adds redundant type conversions for typed constants. For example:

Inlining this call:	func _() { f(c)  }
of this callee:    	func f(i int32) { print(i, i) }; const c int32 = 0
produced:

func _() { print(int32(c), int32(c)) }

Want:

func _() { print(c, c) }

It also results in (somewhat odd) results for duplicable type conversions:

Inlining this call:	func _() { f(I(1))  }
of this callee:    	func f(i I) { print(i, i) }; type I int
produced:

func _() { print(I(I(1)), I(I(1))) }

Want:

func _() { print(I(1), I(1)) }

Fortunately, it does not break the logic, and there is a quick fix (practically, a one-liner). Should I apply it as a part of CL for this issue or move it out into a separate issue?

@adonovan
Copy link
Member

Sure, you can include both changes in the same CL, if both have tests and are mentioned in the CL description.
(I trust you are aware of the reasons the inliner is currently very conservative about adding conversions; see the package doc comments and existing tests if not.)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/588175 mentions this issue: internal/refactor/inline: allow duplicable type conversions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants