Skip to content

cmd/compile: handle subslices in range -> memclear optimization #18908

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
Closed
josharian opened this issue Feb 2, 2017 · 7 comments
Closed

cmd/compile: handle subslices in range -> memclear optimization #18908

josharian opened this issue Feb 2, 2017 · 7 comments
Labels
FrozenDueToAge Performance Suggested Issues that may be good for new contributors looking for work to do.
Milestone
Go1.9Maybe

Comments

@josharian
Copy link
Contributor

josharian commented Feb 2, 2017 •
edited
Loading 

package p

func f(x []byte) {
	for i := range x {
		x[i] = 0
	}
}

func g(x []byte, a, b int) {
	for i := range x[a:b] {
		x[a+i] = 0
	}
}

The core of f gets compiled into a memclear call. The core of g does not, because the current pattern matcher looks for an exact match between the range expression and the LHS of the zeroing statement. We should expand this to allow the range expression to match the LHS of the zeroing statement plus slicing. The generated code should first generate any required bounds checks for the slicing, then call memclear with appropriately adjusted values, taking care if necessary not to generate a pointer to the end of the slice.

If/when this gets implemented, also update some quirky code in the compiler that was written to work around this shortcoming. (For a sample, grep for tail :=. I suspect https://go-review.googlesource.com/36212 will introduce more such code.)

This might be a good moderate-difficulty medium-sized project for someone learning the compiler.

cc @martisch @randall77
@josharian josharian added this to the Go1.9Maybe milestone Feb 2, 2017
@randall77
Copy link
Contributor

The code in g should be x[a+i] = 0, right?

@josharian
Copy link
Contributor Author

Yes. Thanks. :)

@josharian
Copy link
Contributor Author

(Or x[i] = 0 if a is 0, e.g. if the range expression is x[:b] or x[0:b]. I know you know this, just writing out extra details in case someone wants to use this as a starter issue.)

@josharian josharian added the Suggested Issues that may be good for new contributors looking for work to do. label Feb 2, 2017
@minux
Copy link
Member

minux commented Feb 3, 2017 via email

@martisch martisch self-assigned this Feb 3, 2017
@martisch
Copy link
Contributor

martisch commented Feb 3, 2017

Looks like a nice project and i am happy to work on it for 1.9.
I just will not have time for it in the next 3 weeks as i want to get some other CLs wrapped up first. Self assigning to me now but if someone feels strongly about finishing this earlier just reassign.

@martisch
Copy link
Contributor

martisch commented Mar 13, 2017 •
edited
Loading

Had a quick look whats involved and i think this is not as localized a change as i thought.

By the time we get to memclrrange the order pass seems to have moved the slicing into a temporary. So we would need to add a isMemclrrange and intercept that case early by adding a OCLEARRANGE Node and teach order and other walks to not hide the slicing away from memclrrange. But adding that much special casing to write e.g.:

for i := range x[:b] {
	x[i] = 0
}

instead of

y := x[:b]
for i := range y {
	y[i] = 0
}

does not seem to pay for the added complexity in the compiler.

Thoughts or hints how to possible make adding this feature easier (or if i understand the interference from the order pass wrong)?

@josharian
Copy link
Contributor Author

does not seem to pay for the added complexity in the compiler.

I agree. I think we should simply not do this. Thanks for digging into it, @martisch.

@golang golang locked and limited conversation to collaborators Mar 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Performance Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
Go1.9Maybe
Development

No branches or pull requests
5 participants
@josharian @minux @martisch @randall77 @gopherbot