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: different behaviors between short and normal variable declarations #36449

Open
go101 opened this issue Jan 8, 2020 · 6 comments
Open
Milestone

Comments

@go101
Copy link

@go101 go101 commented Jan 8, 2020

This issue is filed per @mdempsky's suggestion.

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

$ go version
go version go1.14beta1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

import "fmt"

func f(p *int) int {
	*p = 123
	return *p
}

func bar() {
	var x int
	y, z := x, f(&x)
	fmt.Println(y, z) // 123 123
}

func baz() {
	var x int
	var y, z = x, f(&x)
	fmt.Println(y, z) // 0 123
}

func main() {
	bar()
	baz()
}

What did you expect to see?

Same outputs for the two functions.

What did you see instead?

Different outputs.

This is an inconsistency in gc, though it looks this doesn't violate Go spec, as some expression evaluation orders in Go are unspecified. But maybe it would be great to make the outputs in this examples consistent.

@go101

This comment has been minimized.

Copy link
Author

@go101 go101 commented Jan 8, 2020

It looks the output of the short declaration case behaves the same as pure assignments:

func foo() {
	var x int
	var y, z int
	y, z = x, f(&x)
	fmt.Println(y, z) // 123 123
}
@toothrot toothrot added this to the Backlog milestone Jan 8, 2020
@toothrot

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Jan 8, 2020

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Jan 8, 2020

It looks like this is because dcl.go:variter emits N:N variable declaration+initialization statements as var v1 t; v1 = e1; var v2 t; v2 = e2; .... This is safe because newly declared variables don't come into scope until after the statement, and short variable declarations must always declare new variables, so we can always initialize them in order.

On the other hand, for v1, v2 := e1, e2 we only require at least one variable to be newly declared. If v1 or v2 is already within scope, then it might appear in the RHS, and we need to be more careful.

As @go101 points out, the discrepancy is still conforming to the Go spec, so it's not something we have to fix.

I suspect the easiest fix (assuming we think it's worth fixing) would be changing variter to emit a single OAS2 node instead of multiple OAS nodes. I'm a little nervous if this has any unintended consequences via n.Name.Defn, but I don't immediately spot anything.

Another question is if it has any effect on compiler performance or on compiled program performance. I haven't looked into these at all yet.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Jan 8, 2020

Another question is if it has any effect on compiler performance or on compiled program performance. I haven't looked into these at all yet.

I tried with a simple patch:

diff --git a/src/cmd/compile/internal/gc/dcl.go b/src/cmd/compile/internal/gc/dcl.go
index 54c6a24df5..8402d818d8 100644
--- a/src/cmd/compile/internal/gc/dcl.go
+++ b/src/cmd/compile/internal/gc/dcl.go
@@ -162,6 +162,22 @@ func variter(vl []*Node, t *Node, el []*Node) []*Node {
                return append(init, as2)
        }

+       if len(vl) > 1 && len(el) == len(vl) && Curfn != nil {
+               as2 := nod(OAS2, nil, nil)
+               as2.List.Set(vl)
+               as2.Rlist.Set(el)
+               for _, v := range vl {
+                       v.Op = ONAME
+                       declare(v, dclcontext)
+                       v.Name.Param.Ntype = t
+                       v.Name.Defn = as2
+                       init = append(init, nod(ODCL, v, nil))
+               }
+               as2.SetColas(true)
+
+               return append(init, as2)
+       }

With this, go tool dist test passes, compilecmp result:

$ compilecmp
03:27:22 compilecmp   master   HEAD
03:27:22 master: cmd/go: adjust heuristics for skipping +incompatible versions
03:27:22 HEAD: xxx
03:27:22 cp <master> /Users/cuonglm/.compilecmp/817afe83578d869b36e8697344bb2d557c86b264
03:27:24 /Users/cuonglm/.compilecmp/817afe83578d869b36e8697344bb2d557c86b264/src/make.bash
03:28:15 cp <HEAD> /Users/cuonglm/.compilecmp/49890043fd25044d53b4167fb4214b7382b855f7
03:28:17 /Users/cuonglm/.compilecmp/49890043fd25044d53b4167fb4214b7382b855f7/src/make.bash
03:29:08 before: /Users/cuonglm/.compilecmp/817afe83578d869b36e8697344bb2d557c86b264
03:29:08 after: /Users/cuonglm/.compilecmp/49890043fd25044d53b4167fb4214b7382b855f7

benchstat -geomean  /var/folders/y4/hs76ltbn7sb66lw_6934kq4m0000gn/T/737143426 /var/folders/y4/hs76ltbn7sb66lw_6934kq4m0000gn/T/497012217
completed   15 of   15, estimated time remaining 0s (eta 3:39AM)
name          old time/op       new time/op       delta
Template            200ms ±13%        191ms ±14%   -4.18%  (p=0.033 n=15+14)
Unicode            85.2ms ±18%       76.7ms ± 8%   -9.94%  (p=0.000 n=15+13)
GoTypes             662ms ±13%        655ms ± 7%     ~     (p=0.652 n=14+15)
Compiler            3.21s ±11%        3.20s ±16%     ~     (p=0.653 n=15+15)
SSA                 10.1s ± 9%        10.2s ± 8%     ~     (p=0.603 n=14+14)
Flate               122ms ±10%        128ms ±23%     ~     (p=0.747 n=14+15)
GoParser            149ms ± 4%        155ms ±14%     ~     (p=0.052 n=13+15)
Reflect             393ms ± 4%        444ms ±23%  +12.77%  (p=0.013 n=13+15)
Tar                 165ms ± 4%        169ms ± 6%   +2.17%  (p=0.029 n=15+14)
XML                 221ms ± 3%        226ms ± 4%   +2.41%  (p=0.033 n=12+12)
LinkCompiler        1.11s ±25%        1.11s ±27%     ~     (p=0.713 n=15+15)
[Geo mean]          435ms             439ms        +1.01%

name          old user-time/op  new user-time/op  delta
Template            264ms ±12%        253ms ± 4%     ~     (p=0.052 n=15+13)
Unicode             136ms ±12%        126ms ± 5%   -7.44%  (p=0.000 n=15+12)
GoTypes             909ms ± 5%        907ms ± 7%     ~     (p=0.786 n=13+15)
Compiler            4.60s ± 9%        4.57s ± 8%     ~     (p=0.813 n=15+14)
SSA                 14.5s ± 4%        14.8s ± 6%     ~     (p=0.185 n=13+15)
Flate               158ms ±11%        166ms ±22%     ~     (p=0.847 n=14+15)
GoParser            190ms ± 4%        194ms ±13%     ~     (p=0.519 n=13+14)
Reflect             548ms ± 4%        610ms ±19%  +11.36%  (p=0.013 n=13+15)
Tar                 214ms ± 3%        214ms ± 4%     ~     (p=0.905 n=14+13)
XML                 290ms ± 3%        295ms ± 6%     ~     (p=0.143 n=12+12)
LinkCompiler        1.18s ±13%        1.17s ±10%     ~     (p=0.653 n=15+15)
[Geo mean]          583ms             587ms        +0.68%

name          old alloc/op      new alloc/op      delta
Template           36.5MB ± 0%       36.5MB ± 0%     ~     (p=0.838 n=15+15)
Unicode            28.4MB ± 0%       28.4MB ± 0%     ~     (p=0.252 n=14+15)
GoTypes             123MB ± 0%        123MB ± 0%     ~     (p=0.174 n=15+15)
Compiler            571MB ± 0%        571MB ± 0%   +0.04%  (p=0.000 n=15+15)
SSA                1.96GB ± 0%       1.96GB ± 0%     ~     (p=0.653 n=15+15)
Flate              22.9MB ± 0%       22.9MB ± 0%     ~     (p=0.363 n=13+15)
GoParser           28.2MB ± 0%       28.2MB ± 0%     ~     (p=0.567 n=15+15)
Reflect            78.9MB ± 0%       78.9MB ± 0%     ~     (p=0.683 n=15+15)
Tar                34.3MB ± 0%       34.3MB ± 0%     ~     (p=0.653 n=15+15)
XML                44.4MB ± 0%       44.4MB ± 0%     ~     (p=0.910 n=14+14)
LinkCompiler        263MB ± 0%        263MB ± 0%     ~     (p=0.148 n=15+15)
[Geo mean]         89.3MB            89.3MB        +0.00%

name          old allocs/op     new allocs/op     delta
Template             372k ± 0%         372k ± 0%     ~     (p=0.814 n=15+15)
Unicode              333k ± 0%         333k ± 0%     ~     (p=0.598 n=14+15)
GoTypes             1.33M ± 0%        1.33M ± 0%     ~     (p=0.281 n=15+15)
Compiler            5.53M ± 0%        5.53M ± 0%   +0.04%  (p=0.000 n=15+14)
SSA                 18.3M ± 0%        18.3M ± 0%     ~     (p=0.713 n=15+15)
Flate                234k ± 0%         234k ± 0%     ~     (p=0.065 n=15+14)
GoParser             305k ± 0%         305k ± 0%     ~     (p=0.060 n=15+15)
Reflect              980k ± 0%         980k ± 0%     ~     (p=0.602 n=15+15)
Tar                  345k ± 0%         345k ± 0%   -0.01%  (p=0.005 n=15+12)
XML                  425k ± 0%         425k ± 0%     ~     (p=0.991 n=14+15)
LinkCompiler         597k ± 0%         597k ± 0%     ~     (p=0.798 n=15+15)
[Geo mean]           813k              813k        +0.00%

name          old text-bytes    new text-bytes    delta
HelloSize           831kB ± 0%        831kB ± 0%     ~     (all equal)

name          old data-bytes    new data-bytes    delta
HelloSize           192kB ± 0%        192kB ± 0%     ~     (all equal)

name          old exe-bytes     new exe-bytes     delta
HelloSize          1.20MB ± 0%       1.20MB ± 0%     ~     (all equal)



file    before    after     Δ       %
compile 24493832  24502024  +8192   +0.033%
total   135247388 135255580 +8192   +0.006%
@go101

This comment has been minimized.

Copy link
Author

@go101 go101 commented Jan 9, 2020

Is it ok for compilers to automatically convert all local normal variable declarations to short variable declarations?

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Jan 9, 2020

Is it ok for compilers to automatically convert all local normal variable declarations to short variable declarations?

That's what my patch above does:

if len(vl) > 1 && len(el) == len(vl) && Curfn != nil {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.