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: 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
Labels
FrozenDueToAge Performance
Milestone
Go1.15

Comments

@randall77
Copy link
Contributor 

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

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

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

@gopherbot
Copy link

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

@josharian
Copy link
Contributor

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.

@josharian
Copy link
Contributor

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

This is fixed with CL 217097.

@golang golang locked and limited conversation to collaborators Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Performance
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
3 participants
@josharian @randall77 @gopherbot