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: prefer to cheaply re-materialize after call site instead of spilling #32255

Open
martisch opened this issue May 26, 2019 · 3 comments
Open

cmd/compile: prefer to cheaply re-materialize after call site instead of spilling #32255

martisch opened this issue May 26, 2019 · 3 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@martisch
Copy link
Contributor

martisch commented May 26, 2019 •
edited

go tip

While working on changing runtime.growslice to not return the length of the new slice (like was done for runtime.makeslice 020a18c) I ran into the issue of not being able to make the compiler rematerialize the new slice length instead of spilling it in

go/src/cmd/compile/internal/gc/ssa.go

Line 2479 in d97bd5d

func (s *state) append(n *Node, inplace bool) *ssa.Value {

There generally seems to be a missed opportunity for the compiler to prefer recomputing a value that is cheap to compute from other values that too need to be loaded after a call.

Hand distilled example:

//go:noinline
func spilltest(s []int, a int) (int, int) {
	b := a + 1
	if b > len(s) {
		somecall()
	}
	return a, b
}

produces:

  0x450380	MOVQ FS:0xfffffff8, CX			
  0x450389	CMPQ 0x10(CX), SP			
  0x45038d	JBE 0x4503d8				
  0x45038f	SUBQ $0x10, SP				
  0x450393	MOVQ BP, 0x8(SP)			
  0x450398	LEAQ 0x8(SP), BP			
  0x45039d	MOVQ 0x30(SP), AX			
  0x4503a2	LEAQ 0x1(AX), CX // first computation of b			
  0x4503a6	MOVQ 0x20(SP), DX			
  0x4503ab	CMPQ DX, CX				
  0x4503ae	JG 0x4503c4				
  0x4503b0	MOVQ AX, 0x38(SP)			
  0x4503b5	MOVQ CX, 0x40(SP)		
  0x4503ba	MOVQ 0x8(SP), BP			
  0x4503bf	ADDQ $0x10, SP				
  0x4503c3	RET					
  0x4503c4	MOVQ CX, 0(SP) // Avoid this spill					
  0x4503c8	CALL main.somecall(SB)			
  0x4503cd	MOVQ 0x30(SP), AX			
  0x4503d2	MOVQ 0(SP), CX // do LEAQ 0x1(AX), CX here instead to compute b
  0x4503d6	JMP 0x4503b0				
  0x4503d8	CALL runtime.morestack_noctxt(SB)	
  0x4503dd	JMP main.spilltest(SB)

Neither

func spilltest(s []int, a int) (int, int) {
	b := a + 1
	if b > len(s) {
		somecall()
		b = a + 1
	}
	return a, b
}

Or

func spilltest(s []int, a int) (int, int) {
	b := a + 1
	if b > len(s) {
		somecall()
	}
        c := a + 1
	return a, c
}

avoids the spilling.

@josharian @randall77 @cherrymui
@martisch martisch added this to the Unplanned milestone May 26, 2019
@josharian
Copy link
Contributor

Does https://go-review.googlesource.com/c/go/+/177041 help?

@martisch
Copy link
Contributor Author

Thanks for the suggestion. Unfortunately it does not look like it changes the example output. It seems that CL is specific to loops while this issue is generally only concerned with re-materialization after calls (does not even need to involve and if).

Before I start digging deeper. Any hints where to best look for making the compiler prefer to materialize cheap ops (e.g. adds) with constants (I guess there is the additional constraint that the added too value is loadable from stack) after calls?

@josharian
Copy link
Contributor

The main levers for that are in tighten and in regalloc. Search for rematerializeable in regalloc, because regalloc does just what you want for rematerializeable values: recompute instead of spill. And as you can see from the CL I linked, tighten may also need to be made aware of regalloc’s policies so that they don’t work at cross purposes.

Worth mentioning that I’m AFK and haven’t at exactly how ssa handles the code you’ve posted.

(I am also reminded of https://go-review.googlesource.com/c/go/+/38448/, which it would be nice to resuscitate, but which is unrelated.)

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 28, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@josharian @martisch @julieqiu @gopherbot