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.

Sign up for GitHub

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

Jump to bottom

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
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@mdempsky
Copy link
Contributor

mdempsky commented Aug 28, 2019 •
edited
Loading

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 mdempsky added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 28, 2019
@mdempsky
Copy link
Contributor Author

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
Copy link
Contributor Author

mdempsky commented Aug 28, 2019 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Member

cuonglm commented Aug 29, 2019

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

@mdempsky
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@cuonglm @t4n6a1ka
cmd/compile: extend ssa.go to handle 1-element array and 1-field struct
c641594 
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>
@golang golang locked and limited conversation to collaborators Sep 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mdempsky @cuonglm @gopherbot @cherrymui