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: merging shifts into loads/stores isn't complete #36223

Closed
randall77 opened this issue Dec 19, 2019 · 6 comments
Closed

cmd/compile: merging shifts into loads/stores isn't complete #36223

randall77 opened this issue Dec 19, 2019 · 6 comments
Assignees
Labels
Milestone

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Dec 19, 2019

func f() int {
	var x [4]int
	for i := 0; i < 4; i++ {
		x[i] = 5
	}
	return x[3]
}

The compiler generates code like this:

	0x0021 00033 (tmp1.go:6)	SHLQ	$3, AX
	0x0025 00037 (ttmp1.go:6)	MOVQ	$5, "".x(SP)(AX*1)

We can use the (AX*8) indexing mode and get rid of the shift.
There is already code to do this in the compiler. However, it doesn't fire in this case, as the shift amount is in the "wrong" arugment slot.
The SSA representation is (MOVQstoreconstidx1 [5] (SHLQconst [3] v) x y). Normally these rules are written to expect the pointer first, then the shifted index, so it does not fire in this case (shifted index first, then pointer).
We should mark ops like these as commutative on the first two args, so the rewrite engine will detect this correctly. There is even a comment in the rule file:

// TODO: mark the MOVXstoreidx1 ops as commutative.  Generates too many rewrite rules at the moment.

I think our rewrite engine now handles commutative ops more efficiently, so possibly not an issue anymore. Investigate, and do it.

@randall77 randall77 added this to the Go1.15 milestone Dec 19, 2019
@randall77 randall77 self-assigned this Dec 19, 2019
@josharian
Copy link
Contributor

@josharian josharian commented Jan 5, 2020

I did some related work in https://go-review.googlesource.com/c/go/+/167089 and https://go-review.googlesource.com/c/go/+/167090. I have better tooling now, so I may try to improve these and try again for 1.15.

However, I don't think we handle commutative ops any more efficiently yet. Do you have plans to work on this? I've been contemplating different approaches.

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Jan 5, 2020

I have not made any plans yet. Go for it.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 7, 2020

Change https://golang.org/cl/213704 mentions this issue: cmd/compile: merge more shifts into stores

@josharian
Copy link
Contributor

@josharian josharian commented Jan 7, 2020

I mailed CLs 213697-213704. They start with cleanup and improvement with the existing commutativity regime, then replace it. There's probably more to do here, but I've blown through the time I allotted to this. I'll file issues for follow-up ideas.

@gopherbot gopherbot closed this in f53f987 Feb 20, 2020
@josharian
Copy link
Contributor

@josharian josharian commented Feb 20, 2020

I write in the CL “may fix” this issue. GitHub ignored the first word. :P

Leaving to @randall77 to decide what else needs to happen to declare this fixed.

@josharian josharian reopened this Feb 20, 2020
@randall77
Copy link
Contributor Author

@randall77 randall77 commented Mar 10, 2020

This is fixed with CL 217097.

@randall77 randall77 closed this Mar 10, 2020
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
3 participants
You can’t perform that action at this time.