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: js-wasm builder broke on "isfat" optimization #33916

Closed
mdempsky opened this issue Aug 28, 2019 · 11 comments
Closed

cmd/compile: js-wasm builder broke on "isfat" optimization #33916

mdempsky opened this issue Aug 28, 2019 · 11 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 28, 2019

https://go-review.googlesource.com/c/go/+/179578 changed plive.go's "isfat" function to treat single element arrays and single field structs as single-value objects rather than multi-value objects. The notable thing about this change is it means for things like:

var x struct { m *blah }
x.m = foo()

the x.m = foo() assignment is considered as clobbering x, whereas before it was treated as just updating a single field, so the variable was considered live.

The CL updated test expectations accordingly, but the js-wasm builder seemed to fail on these, as it still considered the struct live in some cases.

The CL has been reverted, but I think it would be good to understand why only js-wasm failed and land the change again.

/cc @cuonglm

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Aug 28, 2019

I think the problem is that ssa/gen/WasmOps.go doesn't annotate instructions with symEffect.

I'm kind of surprised there aren't more liveness issues on GOARCH=wasm.

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Aug 28, 2019

I see. So in live2.go's bad40, immediately before SSA lowering, we have (v25 is the return value from makemap, and v23 is the mem object returned by it):

v2 = SP
v26 = LocalAddr <*T40> {ret} v2 v23
v27 = OffPtr <*map[int]int> [0] v26
v28 = Store <mem>{map[int]int} v27 v25 v23

On most architectures (I tested amd64 and arm64, and I expect others are the same), this gets lowered to something like:

v2 = SP
v8 = MOVQstore <mem> {ret} v2 v25 v23

Critically, plive.go is able to recognize than this MOVQ clobbers ret because MOVQstore is annotated with symEffect: "write" in gen/AMD64Ops.go.

However, on wasm, this gets lowered to:

v2 = SP
v26 = LoweredAddr <*T40> {ret} v2
v28 = I64Store <mem> [0] v26 v25 v23

which I think is roughly comparable to something like:

v2 = SP
v26 = LEAQ <*T40> {ret} v2
v28 = MOVQstore <mem> [0] v26 v25 v23

That is, the address computation is split out separately, and plive.go can't figure out the store is clobbering anything.

I think wasm should switch to support SymOffset for its load and store operations like other GOARCHes.

/cc @randall77 @cherrymui @neelance

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Aug 28, 2019

I think wasm should switch to support SymOffset for its load and store operations like other GOARCHes.

It seems like the reason this isn't a bigger issue for wasm is that gc/ssa.go inserts OpVarDef instructions around assignments where possible. This issue came up because it introduced new cases where plive.go was able to detect a variable was dead where gc/ssa.go didn't notice.

So an alternative fix here would be to extend gc/ssa.go to incorporate the "isfat" logic for figuring out when it can insert OpVarDef.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Aug 29, 2019

@mdempsky IIRC, buildssa emits OpVarDef regardless of variable is fat or not.

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Aug 29, 2019

IIRC, buildssa emits OpVarDef regardless of variable is fat or not.

For assignments like x = ... it does, but I don't believe that's the case for subvariable assignments like x.m = ....

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Aug 31, 2019

I think wasm should switch to support SymOffset for its load and store operations like other GOARCHes.

I think this makes sense, at least for local variables.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Sep 3, 2019

So an alternative fix here would be to extend gc/ssa.go to incorporate the "isfat" logic for figuring out when it can insert OpVarDef.

@mdempsky can you describe it more details?

Looking at SSA output, I see OpVarDef was inserted when ret declared. Also where must we incorporate the "isfat" logic?

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Sep 3, 2019

@mdempsky can you describe it more details?

gc/ssa.go emits OpVarDef when an assignments overwrites the entire variable. Currently it does this when an object is declared, and also immediately before assignments that overwrite the entire object (eg, x = ...). Note: OpVarDef can be emitted multiple times for a single variable.

The alternative suggestion I was making was that gc/ssa.go should also recognize x.m = ... assignments where x's type is not "fat".

Also where must we incorporate the "isfat" logic?

I would expect it would just be a tweak to the existing OpVarDef logic in builder.assign.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Sep 3, 2019

@mdempsky can you describe it more details?

gc/ssa.go emits OpVarDef when an assignments overwrites the entire variable. Currently it does this when an object is declared, and also immediately before assignments that overwrite the entire object (eg, x = ...). Note: OpVarDef can be emitted multiple times for a single variable.

The alternative suggestion I was making was that gc/ssa.go should also recognize x.m = ... assignments where x's type is not "fat".

Also where must we incorporate the "isfat" logic?

I would expect it would just be a tweak to the existing OpVarDef logic in builder.assign.

Hmm, the same as I did, but still no luck, maybe I check the wrong place, let me re-check.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 3, 2019

Change https://golang.org/cl/192978 mentions this issue: Revert "Revert "cmd/compile: make isfat handle 1-element array, 1-field struct""

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 3, 2019

Change https://golang.org/cl/192979 mentions this issue: cmd/compile: extend ssa.go to incorporate the "isfat" logic

@gopherbot gopherbot closed this in d2f958d Sep 3, 2019
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Assinging to 1-element array/1-field struct variable is considered clobbering
the whole variable. By emitting OpVarDef in this case, liveness analysis
can now know the variable is redefined.

Also, the isfat is not necessary anymore, and will be removed in follow up CL.

Fixes golang#33916

Change-Id: Iece0d90b05273f333d59d6ee5b12ee7dc71908c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/192979
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
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.