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
Comments
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:
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. |
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). |
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! :( |
Change https://golang.org/cl/109776 mentions this issue: |
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>
Punting to 1.13, too late for anything major in 1.12. |
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
The text was updated successfully, but these errors were encountered: