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/internal/gc: ssa.go contains some Intel-flavored idioms (and maybe an error) #15492

Closed
dr2chase opened this issue Apr 29, 2016 · 6 comments
Closed

cmd/compile/internal/gc: ssa.go contains some Intel-flavored idioms (and maybe an error) #15492

dr2chase opened this issue Apr 29, 2016 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@dr2chase
Copy link
Contributor

Cherry spotted this, came to me for a sanity check. In insertWBmove and insertWBstore this code appears:

    flagaddr := s.newValue1A(ssa.OpAddr, Ptrto(Types[TUINT32]), aux, s.sb)
    // TODO: select the .enabled field. It is currently first, so not needed for now.
    // Load word, test byte, avoiding partial register write from load byte.
    flag := s.newValue2(ssa.OpLoad, Types[TUINT32], flagaddr, s.mem())
    flag = s.newValue1(ssa.OpTrunc64to8, Types[TBOOL], flag)

Note the 32-bit-load followed by a Trunc64to8 -- as far as we know, not a bug, but not quite what was intended. The Intel flavoring is the wide load followed by truncate; on other platforms a load byte is going to work just fine because they lack the partial-register craziness.

Since this is in writebarrier code, we actually care.
@dr2chase dr2chase added this to the Go1.8 milestone Apr 29, 2016
@randall77
Copy link
Contributor

That should be Trunc32to8.

We could just use all 32-bit operations on amd64, no reason to use the byte instructions.
MOVL foo(SB), AX
TESTL AX, AX
JNE dowritebarrier

@josharian
Copy link
Contributor

Related (alternative fix): #15245

@cherrymui
Copy link
Member

Thanks @randall77 and @josharian. As we may have alternative fix, I leave it unchanged on amd64 for now, except Trunc64to8 -> Trunc32to8.

@gopherbot
Copy link

CL https://golang.org/cl/22653 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/22855 mentions this issue.

gopherbot pushed a commit that referenced this issue May 10, 2016
@cherrymui
[dev.ssa] cmd/compile/internal/gc, runtime: use 32-bit load for write…
fdc4a96 
…Barrier check

Use 32-bit load for writeBarrier check on all architectures.
Padding added to runtime structure.

Updates #15365, #15492.

Change-Id: I5d3dadf8609923fe0fe4fcb384a418b7b9624998
Reviewed-on: https://go-review.googlesource.com/22855
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@quentinmit quentinmit changed the title gc/ssa.go contains some Intel-flavored idioms (and maybe an error) cmd/compile/internal/gc: ssa.go contains some Intel-flavored idioms (and maybe an error) Oct 5, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 5, 2016
@randall77
Copy link
Contributor

I believe this is fixed. On all archs we now use 32-bit operations.

@golang golang locked and limited conversation to collaborators Oct 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
6 participants
@josharian @quentinmit @dr2chase @randall77 @gopherbot @cherrymui