proposal: unsafe: add Unreachable #30582
Comments
|
What happens if the assertion does not hold? |
|
The compiler may generate code that does all sorts of awful things. It is decidedly unsafe. |
|
To me, this sounds like "Assume". I would think "Assert" means if the assertion fails, the program crashes (more like the C assert function). |
josharian
changed the title
|
@cherrymui thanks, proposal updated. |
|
Another approach would be if p == nil {
unsafe.Unreachable()
} else {
// Here the compiler knows that p != nil.
}and then at some point during code generation the compiler removes the |
|
@ianlancetaylor I like it. One nice thing about that is that it mirrors the not-unsafe way to write it: if p != nil {
panic("unreachable")
}becomes if p != nil {
unsafe.Unreachable()
}And the specification is simpler. The only downside I see is that it doesn't fit on one line. :P It also doesn't scream unsafe at me the way |
josharian
changed the title
|
What happens if you ask the compiler to assume something it knows is false? Like something that reduces to |
Then you're in UB world: It depends on the compiler and the details. I'm guessing that in practice, with cmd/compile right now, probably not too much. With gccgo, you probably start spewing random numbers until you generate the kubernetes code base. |
|
If the compiler doesn't know that |
Perhaps optionally? I think that a compiler that simply ignored |
|
I like this a lot for my code, I'm terrified of what happens if other people have it. |
|
I'm not sure we should go down this road. This is basically a "performance annotation" and it's one more thing everyone needs to understand. I'm happy to give up 5% in performance to never have to make (or see) any such annotations. There's a lot of other annotations people could want. If we do this I like Ian's API. |
@josharian could you clarify what part is expensive? The added code size? Or perhaps making some small functions non-inlineable with the current compiler? Another idea that comes to mind is if the compiler treated |
|
Change https://golang.org/cl/165358 mentions this issue: |
|
I wrote a rudimentary implementation so that I could experiment with it: https://golang.org/cl/165358
Some data points, for Going from my recently-optimized CLs to adding a few unsafe.Unreachable annotations: Going from regular to an unrolled loop, which requires yet more annotations: This gets us within striking distance of the hand-optimized assembly for small n. Going from my unrolled Go+Unreachable code to assembly: And the main difference now is that the hand-rolled assembly can do I plan to do the ADC upgrade anyway. I'll report back once I've done it; might be a few days. None of the |
|
And if you want to see what the simple, hinted code looks like: func addVV_g(z, x, y []Word) (c Word) {
for i := 0; i < len(z); i++ {
if i >= len(x) || i >= len(y) {
// The calling code ensures that x and y are no longer than z.
// The compiler doesn't see this; this hint prevents bounds
// checks in the bits.Add call below.
unsafe.Unreachable()
}
zi, cc := bits.Add(uint(x[i]), uint(y[i]), uint(c))
z[i] = Word(zi)
c = Word(cc)
}
return
} |
Fair enough. That said, I think this is plausibly qualitatively different:
|
|
I'd be interested to see the benchmarks for func addVV_g(z, x, y []Word) (c Word) {
n := len(z)
x, y = x[:n:n], y[:n:n]
for i := 0; i < n; i++ {
zi, cc := bits.Add(uint(x[i]), uint(y[i]), uint(c))
z[i] = Word(zi)
c = Word(cc)
}
return
}I suspect the code using Also, if the compiler inlined code using I can see annotations like EDIT: dropped the length check panics, I don't think they are necessary for this use case and with them the function doesn't work when |
|
I did a few benchmarks on the generic code to see what I got. Hoisting the bounds check on y seemed to make very little difference. Which is interesting because they did remove the bounds check from the loop, implying the critical path is the carry chain (I am using an i5 3570k). Secondly, I tried applying David's for loop inlining experiment CL 148777. I still applied the bounds check hoisting change to get rid of the I suspect the slightly slower speed limit is down to alignment noise or a similar effect. Patch to remove range statement and bounds check on y in the loop:--- a/src/math/big/arith.go
+++ b/src/math/big/arith.go
@@ -54,7 +54,9 @@ func divWW_g(u1, u0, v Word) (q, r Word) {
// The resulting carry c is either 0 or 1.
func addVV_g(z, x, y []Word) (c Word) {
- for i := range x[:len(z)] {
+ n := len(z)
+ x, y = x[:n:n], y[:n:n]
+ for i := 0; i < n; i++ {
zi, cc := bits.Add(uint(x[i]), uint(y[i]), uint(c))
z[i] = Word(zi)
c = Word(cc) |
ianlancetaylor
added
the
NeedsDecision
ianlancetaylor
changed the title
DO NOT SUBMIT This is a rudimentary implementation, so that I can experiment with it. Updates golang#30582 Change-Id: I2e3a9e776490a9b01d4f166014b133625e419418
DO NOT SUBMIT This is a rudimentary implementation, so that I can experiment with it. Updates golang#30582 Change-Id: I2e3a9e776490a9b01d4f166014b133625e419418
DO NOT SUBMIT This is a rudimentary implementation, so that I can experiment with it. Updates golang#30582 Change-Id: I2e3a9e776490a9b01d4f166014b133625e419418
That seems dangerous to me: it adds a large space of undefined behavior (that is, any program that violates the assumption), but no way to validate that the program does not actually exhibit that behavior (contrast To me, the biggest advantage (by far!) of undefined behavior is that it can be sanitized: unlike a Go So instead, I would propose that the expression always be evaluated, and the result discarded except in sanitized (perhaps race-detector-enabled?) builds. That makes it possible to optimize away arithmetic expressions and pure functions (by far the common use-cases), but allows a sanitizer to verify the assumptions — without producing inconsistent behavior if the expression has side-effects. |
gopherbot
added
the
NeedsDecision
|
Interesting, but the idea that there should be a difference between debug mode and release mode is false. I have spent years doing life critical software in a large, famous, company, and there the rule was the production binary must be the tested binary, which almost always meant the binary with debug information available, in order to be able to do postmortem analysis in case of a crash. Nevertheless |
We already have this: |
|
|
|
If I write your example above slightly differently (and encapsulate into a complete package so I can compile it): package main
import "math/bits"
type Word uint64
func addVV_g(z, x, y []Word) (c Word) {
if len(x) != len(z) || len(y) != len(z) {
panic("vector lengths don't match")
}
// lengths of x, y, z are the same
for i := 0; i < len(z); i++ {
zi, cc := bits.Add(uint(x[i]), uint(y[i]), uint(c))
z[i] = Word(zi)
c = Word(cc)
}
return
}
func main() {}It looks like the compiler is already smart enough to do the right thing. The inner loop appears to be: or, copying from godbolt.org: So at least for this code we may not have to do anything. |
ianlancetaylor
changed the title
ianlancetaylor
removed
v2
|
This proposal has been added to the active column of the proposals project |
|
@griesemer points out that we added unsafe to provide the ability to do things that the language could not express but that needed to be available, specifically type-unsafe conversions. unsafe.Unreachable is different: it's purely a compiler optimization, not giving new expressive power. I worry a lot about debugability of buggy code, especially after years of suffering C and C++ compilers and "undefined behavior". In general Go's approach has been to prioritize safe code execution over absolute raw performance. C/C++ compilers have shown us where we end up when the compiler assumes things like "array indexes are always in bound" or "pointers are never null". It's true that unsafe.Unreachable would only produce more limited effects, since "always" and "never" are replaced by "in this specific instance". Even so, I fear that adding unsafe.Unreachable will encourage overuse for the sake of performance and lead to code that misbehaves in mysterious ways. Separately, the fact that the compiler is not smart enough to optimize away certain checks creates a constructive tension on developers that makes us search out new analyses or idioms that optimize better but are still safe. An example of this is the |
|
Based on the discussion above, this proposal seems like a likely decline. |
|
No change in consensus, so declined. |
When optimizing code, I often run up against cases in which the compiler is missing a key fact. Sometimes it could but does not infer it; sometimes there’s no reasonable way for the compiler to know.
In those cases, there is nothing to do but drop to assembly. (In normal code you could write
if !x { panic }, but in many of these cases, that is prohibitively expense.)I propose that we add unsafe.Assume. It accepts a boolean expression. The expression is typechecked but never evaluated. However, the compiler may assume that it evaluates to true when compiling other code.
I imagine the most common uses would be things like
unsafe.Assume(p != nil),unsafe.Assume(0 <= i && i < len(s)), andunsafe.Assume(x < 64), for nil checks, bounds checks, and shift amounts, respectively.The text was updated successfully, but these errors were encountered: