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: use bounds from prove pass to simplify shifts #25087

Open
josharian opened this Issue Apr 25, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@josharian
Contributor

josharian commented Apr 25, 2018

Shifts generally require extra code to correctly handle shifting by more than the width of the value.

We have a bunch of ad hoc rewrite rules to detect and handle this. But the prove pass might be able to do a better job of this, and more simply too.

CL 109358 uses the prove pass to simplify CtzNN. Maybe we should do something similar for shifts. (And anything else?)

cc @rasky @randall77

@josharian josharian added this to the Go1.12 milestone Apr 25, 2018

@rasky

This comment has been minimized.

Member

rasky commented Apr 26, 2018

The main issue with implementing this is that prove only learn relations from "if" statements. Your CL about adding CtzNonZero was easy because you were looking into using some information that was being stated as part of a condition, so it was already there and only had to be looked up.

In this case, the majority of code that computes a non constant shift amount probably never goes through an if statement. I don't think it's common to see code like:

if shift >= 64 { ... return }
a <<= shift

which is where prove could be useful today.

Having this kind of knowledge would require a full VRP pass, where you go through every instruction in the function and check whether you can learn something about each and every SSA var. I think @dr2chase attempted this in SSA even before SSA was merged.

@dr2chase

This comment has been minimized.

Contributor

dr2chase commented Apr 26, 2018

This is correct (that I took a shot at this), it might be worth revisiting, maybe integrate it into a rewrite pass (e.g., do rewrites in some variant of reverse post order and/or dominator tree visit so that predecessors are all visited, and carry obvious conditions forward and apply them in rewrites).

@josharian

This comment has been minimized.

Contributor

josharian commented Apr 26, 2018

The main issue with implementing this is that prove only learn relations from "if" statements.

Did a quick hack-up of this, and it turns out there are some if statements that bound shifts. 35 in std+cmd, and more if #25115 is fixed. Will try to polish and mail. So many things to try to get mailed before the freeze! :(

@gopherbot

This comment has been minimized.

gopherbot commented Apr 27, 2018

Change https://golang.org/cl/109776 mentions this issue: cmd/compile: simplify shifts using bounds from prove pass

gopherbot pushed a commit that referenced this issue Apr 29, 2018

cmd/compile: simplify shifts using bounds from prove pass
The prove pass sometimes has bounds information
that later rewrite passes do not.

Use this information to mark shifts as bounded,
and then use that information to generate better code on amd64.
It may prove to be helpful on other architectures, too.

While here, coalesce the existing shift lowering rules.

This triggers 35 times building std+cmd. The full list is below.

Here's an example from runtime.heapBitsSetType:

			if nb < 8 {
				b |= uintptr(*p) << nb
				p = add1(p)
			} else {
				nb -= 8
			}

We now generate better code on amd64 for that left shift.

Updates #25087

vendor/golang_org/x/crypto/curve25519/mont25519_amd64.go:48:20: Proved Rsh8Ux64 bounded
runtime/mbitmap.go:1252:22: Proved Lsh64x64 bounded
runtime/mbitmap.go:1265:16: Proved Lsh64x64 bounded
runtime/mbitmap.go:1275:28: Proved Lsh64x64 bounded
runtime/mbitmap.go:1645:25: Proved Lsh64x64 bounded
runtime/mbitmap.go:1663:25: Proved Lsh64x64 bounded
runtime/mbitmap.go:1808:41: Proved Lsh64x64 bounded
runtime/mbitmap.go:1831:49: Proved Lsh64x64 bounded
syscall/route_bsd.go:227:23: Proved Lsh32x64 bounded
syscall/route_bsd.go:295:23: Proved Lsh32x64 bounded
syscall/route_darwin.go:40:23: Proved Lsh32x64 bounded
compress/bzip2/bzip2.go:384:26: Proved Lsh64x16 bounded
vendor/golang_org/x/net/route/address.go:370:14: Proved Lsh64x64 bounded
compress/flate/inflate.go:201:54: Proved Lsh64x64 bounded
math/big/prime.go:50:25: Proved Lsh64x64 bounded
vendor/golang_org/x/crypto/cryptobyte/asn1.go:464:43: Proved Lsh8x8 bounded
net/ip.go:87:21: Proved Rsh8Ux64 bounded
cmd/internal/goobj/read.go:267:23: Proved Lsh64x64 bounded
cmd/vendor/golang.org/x/arch/arm64/arm64asm/decode.go:534:27: Proved Lsh32x32 bounded
cmd/vendor/golang.org/x/arch/arm64/arm64asm/decode.go:544:27: Proved Lsh32x32 bounded
cmd/internal/obj/arm/asm5.go:1044:16: Proved Lsh32x64 bounded
cmd/internal/obj/arm/asm5.go:1065:10: Proved Lsh32x32 bounded
cmd/internal/obj/mips/obj0.go:1311:21: Proved Lsh32x64 bounded
cmd/compile/internal/syntax/scanner.go:352:23: Proved Lsh64x64 bounded
go/types/expr.go:222:36: Proved Lsh64x64 bounded
crypto/x509/x509.go:1626:9: Proved Rsh8Ux64 bounded
cmd/link/internal/loadelf/ldelf.go:823:22: Proved Lsh8x64 bounded
net/http/h2_bundle.go:1470:17: Proved Lsh8x8 bounded
net/http/h2_bundle.go:1477:46: Proved Lsh8x8 bounded
net/http/h2_bundle.go:1481:31: Proved Lsh64x8 bounded
cmd/compile/internal/ssa/rewriteARM64.go:18759:17: Proved Lsh64x64 bounded
cmd/compile/internal/ssa/sparsemap.go:70:23: Proved Lsh32x64 bounded
cmd/compile/internal/ssa/sparsemap.go:73:45: Proved Lsh32x64 bounded

Change-Id: I58bb72f3e6f12f6ac69be633ea7222c245438142
Reviewed-on: https://go-review.googlesource.com/109776
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
@randall77

This comment has been minimized.

Contributor

randall77 commented Dec 12, 2018

Punting to 1.13, too late for anything major in 1.12.

@randall77 randall77 modified the milestones: Go1.12, Go1.13 Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment