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, cmd/internal/obj/ppc64: generate the likely/unlikely bit on branches for ppc64x #17235

Closed
laboger opened this issue Sep 26, 2016 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@laboger
Copy link
Contributor

laboger commented Sep 26, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version devel +375092b Mon Sep 26 15:46:44 2016 +0000 linux/ppc64le

What operating system and processor architecture are you using (go env)?

Ubuntu 16.04

What did you do?

Perused generated ppc64x code.

What did you expect to see?

Branch likely flag set for many commonly generated if-then-else sequences when the 'then' block should rarely be taken.

What did you see instead?

I don't see the likely/unlikely flag set it by golang generated code except in asm files.

The asm9.go file has support to set the likely/unlikely flag but the branch asm opcodes don't currently have an operand combination which indicates when it should be done.

I plan to add support for an operand with the branch that indicates when to set likely/unlikely, then update some of the sequences that should have the flag set. This includes morestack_noctxt in the prolog, and the runtime checks that result in panics once I figure out where those are.

@randall77
Copy link
Contributor

cmd/compile/internal/ssa/block.go:Block has a branch prediction field (called Likely).
You can use that in cmd/compile/internal/ppc64/ssa.go to populate a branch Prog with the right data.
For amd64 we use Prog.From=={Const,0} for unlikely taken and {Const,1} for likely taken. Use that encoding for PPC as well if it makes sense.
See cmd/compile/internal/amd64/ssa.go for an example of how to do that.

@quentinmit quentinmit changed the title cmd/compile, cmd/internal/obj/ppc64 generate the likely/unlikely bit on branches for ppc64x cmd/compile, cmd/internal/obj/ppc64: generate the likely/unlikely bit on branches for ppc64x Oct 3, 2016
@quentinmit quentinmit added this to the Go1.8 milestone Oct 3, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 3, 2016
@laboger
Copy link
Contributor Author

laboger commented Oct 14, 2016

I will not be submitting a change for this in Go 1.8. The performance testing I've done for this had mixed results and will require more investigation to determine if it is worth it.

@rsc rsc modified the milestones: Go1.9, Go1.8 Oct 21, 2016
@laboger
Copy link
Contributor Author

laboger commented Jan 18, 2017

I've done some investigation on this, setting the likely bit based on the end of block information. However that does not always provide an improvement, it varies quite a bit and in many cases gets worse. I have not investigated further to try and determine if the information provided is not dependable for ppc64x or if there is some other reason.

If I were to set this at all, I'd prefer to only set it in the obvious cases, such as when there is an if check for a panic condition and then the panic path marked as unlikely. Not sure what the best way is to determine that.

@dr2chase
Copy link
Contributor

Thanks for the research. I've done the analysis in the past (other languages, other compilers) to do the "this will certainly fail, so it's certainly not a critical path" likeliness check, and I think we could do that here, too.

I have an item on my to-do list to write up a list of little useful projects, and that's one of them.
(Tentative list: improve spill location in register allocation; improve escape analysis w/ interprocedural use info for interface-typed call args; spec out loop unrolling within SSA form for both optimization and the reschedule check; this finer-grained "likely" information. I think Keith has a similar list).

@randall77 randall77 modified the milestones: Go1.10, Go1.9 May 31, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 28, 2017
@bradfitz bradfitz modified the milestones: Go1.11, Unplanned May 18, 2018
@laboger
Copy link
Contributor Author

laboger commented Jul 20, 2020

I'm going to close this. It looks like the cases I was concerned with are now marked with the correct likely flag.

@laboger laboger closed this as completed Jul 20, 2020
@golang golang locked and limited conversation to collaborators Jul 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

7 participants