Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(251)

Issue 6854063: code review 6854063: cmd/5g: use MOVB for fixed array nil check (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by dfc
Modified:
9 years, 6 months ago
Reviewers:
CC:
remyoudompheng, minux1, rsc, golang-dev
Visibility:
Public.

Description

cmd/5g: use MOVB for fixed array nil check Fixes issue 4396. For fixed arrays larger than the unmapped page, agenr would general a nil check by loading the first word of the array. However there is no requirement for the first element of a byte array to be word aligned, so this check causes a trap on ARMv5 hardware (ARMv6 since relaxed that restriction, but it probably still comes at a cost). Switching the check to MOVB ensures alignment is not an issue. This check is only invoked in a few places in the code where large fixed arrays are embedded into structs, compress/lzw is the biggest offender, and switching to MOVB has no observable performance penalty. Thanks to Rémy and Daniel Morsing for helping me debug this on IRC last night.

Patch Set 1 #

Patch Set 2 : diff -r 11094b97d92a https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 11094b97d92a https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 11094b97d92a https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 11094b97d92a https://code.google.com/p/go #

Patch Set 6 : diff -r 11094b97d92a https://code.google.com/p/go #

Total comments: 5

Patch Set 7 : diff -r bbc0024af4a4 https://code.google.com/p/go #

Total comments: 3

Patch Set 8 : diff -r db11b6a8c8f9 https://code.google.com/p/go #

Total comments: 2

Patch Set 9 : diff -r db11b6a8c8f9 https://code.google.com/p/go #

Total comments: 2

Patch Set 10 : diff -r 3fc28eec1b38 https://code.google.com/p/go #

Patch Set 11 : diff -r 3fc28eec1b38 https://code.google.com/p/go #

Patch Set 12 : diff -r 025b9d070a85 https://code.google.com/p/go #

Total comments: 9

Patch Set 13 : diff -r 025b9d070a85 https://code.google.com/p/go #

Patch Set 14 : diff -r 6e0e4077f488 https://go.googlecode.com/hg/ #

Patch Set 15 : diff -r 6e0e4077f488 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -12 lines) Patch
M src/cmd/5g/cgen.c View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +19 lines, -12 lines 0 comments Download
A test/fixedbugs/issue4396a.go View 1 2 3 4 5 6 7 8 9 1 chunk +27 lines, -0 lines 0 comments Download
A test/fixedbugs/issue4396b.go View 1 2 3 4 5 6 7 8 9 1 chunk +29 lines, -0 lines 0 comments Download

Messages

Total messages: 25
dfc
Hello remyoudompheng@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
9 years, 7 months ago (2012-11-17 22:12:07 UTC) #1
remyoudompheng
6g uses TESTB for that purpose, so LGTM I think 6g calls gins with nodes ...
9 years, 7 months ago (2012-11-17 22:48:04 UTC) #2
dfc
PTAL. I tried to make the check less cheaty and in the process it now ...
9 years, 7 months ago (2012-11-18 04:20:26 UTC) #3
dfc
Hello remyoudompheng@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-18 04:21:56 UTC) #4
minux1
https://codereview.appspot.com/6854063/diff/2002/src/cmd/5g/cgen.c File src/cmd/5g/cgen.c (right): https://codereview.appspot.com/6854063/diff/2002/src/cmd/5g/cgen.c#newcode953 src/cmd/5g/cgen.c:953: gmove(&n3, &n4); i think this suffices: regalloc(&n4, types[TUINT8], N); ...
9 years, 7 months ago (2012-11-18 08:15:41 UTC) #5
dfc
Thanks for the suggestion, I'll try it. As for the test, the types come from ...
9 years, 7 months ago (2012-11-18 08:24:29 UTC) #6
dfc
PTAL https://codereview.appspot.com/6854063/diff/2002/src/cmd/5g/cgen.c File src/cmd/5g/cgen.c (right): https://codereview.appspot.com/6854063/diff/2002/src/cmd/5g/cgen.c#newcode953 src/cmd/5g/cgen.c:953: gmove(&n3, &n4); On 2012/11/18 08:15:41, minux wrote: > ...
9 years, 7 months ago (2012-11-18 09:57:01 UTC) #7
minux1
you might also need to change gsubr.c ine 1196 and 1969. On 2012/11/18 09:57:01, dfc ...
9 years, 7 months ago (2012-11-18 10:21:29 UTC) #8
remyoudompheng
It might be useful to use checkref (defined in gsubr.c) to the task.
9 years, 7 months ago (2012-11-18 10:30:43 UTC) #9
dfc
I'd like to leave this proposal as is for the moment, so I can enable ...
9 years, 7 months ago (2012-11-18 11:00:10 UTC) #10
remyoudompheng
http://codereview.appspot.com/6854063/diff/5003/src/cmd/5g/cgen.c File src/cmd/5g/cgen.c (right): http://codereview.appspot.com/6854063/diff/5003/src/cmd/5g/cgen.c#newcode957 src/cmd/5g/cgen.c:957: regalloc(&tmp, types[TUINT8], N); i would use &n4 as third ...
9 years, 7 months ago (2012-11-24 19:43:45 UTC) #11
dfc
https://codereview.appspot.com/6854063/diff/5003/src/cmd/5g/cgen.c File src/cmd/5g/cgen.c (right): https://codereview.appspot.com/6854063/diff/5003/src/cmd/5g/cgen.c#newcode957 src/cmd/5g/cgen.c:957: regalloc(&tmp, types[TUINT8], N); On 2012/11/24 19:43:45, remyoudompheng wrote: > ...
9 years, 7 months ago (2012-11-25 11:59:15 UTC) #12
remyoudompheng
https://codereview.appspot.com/6854063/diff/5003/src/cmd/5g/cgen.c File src/cmd/5g/cgen.c (right): https://codereview.appspot.com/6854063/diff/5003/src/cmd/5g/cgen.c#newcode957 src/cmd/5g/cgen.c:957: regalloc(&tmp, types[TUINT8], N); On 2012/11/25 11:59:15, dfc wrote: > ...
9 years, 7 months ago (2012-11-25 12:06:03 UTC) #13
dfc
Hello remyoudompheng@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-25 12:11:57 UTC) #14
remyoudompheng
http://codereview.appspot.com/6854063/diff/4002/test/fixedbugs/issue4396.go File test/fixedbugs/issue4396.go (right): http://codereview.appspot.com/6854063/diff/4002/test/fixedbugs/issue4396.go#newcode24 test/fixedbugs/issue4396.go:24: can you add the following test: type T struct ...
9 years, 7 months ago (2012-11-25 12:21:34 UTC) #15
dfc
Hello remyoudompheng@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-11-25 12:38:15 UTC) #16
remyoudompheng
http://codereview.appspot.com/6854063/diff/4003/src/cmd/5g/cgen.c File src/cmd/5g/cgen.c (right): http://codereview.appspot.com/6854063/diff/4003/src/cmd/5g/cgen.c#newcode783 src/cmd/5g/cgen.c:783: gmove(a, &n1); hum, the test should not have passed ...
9 years, 7 months ago (2012-11-25 12:52:44 UTC) #17
dfc
I need to test on some arm5 hardware on Monday. On 25 Nov 2012 23:52, ...
9 years, 7 months ago (2012-11-25 12:54:51 UTC) #18
rsc
LGTM once you're happy about the testing https://codereview.appspot.com/6854063/diff/4003/test/fixedbugs/issue4396.go File test/fixedbugs/issue4396.go (right): https://codereview.appspot.com/6854063/diff/4003/test/fixedbugs/issue4396.go#newcode24 test/fixedbugs/issue4396.go:24: T [4096]byte ...
9 years, 7 months ago (2012-11-26 17:23:05 UTC) #19
dfc
Hello remyoudompheng@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-12-04 11:07:28 UTC) #20
remyoudompheng
https://codereview.appspot.com/6854063/diff/15001/src/cmd/5g/cgen.c File src/cmd/5g/cgen.c (right): https://codereview.appspot.com/6854063/diff/15001/src/cmd/5g/cgen.c#newcode707 src/cmd/5g/cgen.c:707: n2.op = OINDREG; s/n2/n1 on these three lines https://codereview.appspot.com/6854063/diff/15001/src/cmd/5g/cgen.c#newcode710 ...
9 years, 7 months ago (2012-12-04 11:12:41 UTC) #21
dfc
Thanks Remy, testing now. https://codereview.appspot.com/6854063/diff/4002/test/fixedbugs/issue4396.go File test/fixedbugs/issue4396.go (right): https://codereview.appspot.com/6854063/diff/4002/test/fixedbugs/issue4396.go#newcode24 test/fixedbugs/issue4396.go:24: On 2012/11/25 12:21:34, remyoudompheng wrote: ...
9 years, 7 months ago (2012-12-04 11:17:54 UTC) #22
dfc
Hello remyoudompheng@gmail.com, minux.ma@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 7 months ago (2012-12-04 11:44:48 UTC) #23
dfc
No significant performance impact from this patch linux/arm, OMAP4 pandaboard. benchmark old ns/op new ns/op ...
9 years, 7 months ago (2012-12-05 04:51:08 UTC) #24
dfc
9 years, 7 months ago (2012-12-05 21:01:51 UTC) #25
*** Submitted as https://code.google.com/p/go/source/detail?r=5eac1a2d6fc3 ***

cmd/5g: use MOVB for fixed array nil check

Fixes issue 4396.

For fixed arrays larger than the unmapped page, agenr would general a nil check
by loading the first word of the array. However there is no requirement for the
first element of a byte array to be word aligned, so this check causes a trap on
ARMv5 hardware (ARMv6 since relaxed that restriction, but it probably still
comes at a cost).

Switching the check to MOVB ensures alignment is not an issue. This check is
only invoked in a few places in the code where large fixed arrays are embedded
into structs, compress/lzw is the biggest offender, and switching to MOVB has no
observable performance penalty.

Thanks to Rémy and Daniel Morsing for helping me debug this on IRC last night.

R=remyoudompheng, minux.ma, rsc
CC=golang-dev
https://codereview.appspot.com/6854063
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b