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

Issue 12424043: code review 12424043: cmd/5c, cmd/5g, cmd/5l: turn MOVB, MOVH into plain move... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 7 months ago by remyoudompheng
Modified:
11 years ago
Reviewers:
rsc, bradfitz
CC:
rsc, dave_cheney.net, bradfitz, golang-dev
Visibility:
Public.

Description

cmd/5c, cmd/5g, cmd/5l: turn MOVB, MOVH into plain moves, optimize short arithmetic. Pseudo-instructions MOVBS and MOVHS are used to clarify the semantics of short integers vs. registers: * 8-bit and 16-bit values in registers are assumed to always be zero-extended or sign-extended depending on their type. * MOVB is truncation or move of an already extended value between registers. * MOVBU enforces zero-extension at the destination (register). * MOVBS enforces sign-extension at the destination (register). And similarly for MOVH/MOVS/MOVHU. The linker is adapted to assemble MOVB and MOVH to an ordinary mov. Also a peephole pass in 5g that aims at eliminating redundant zero/sign extensions is improved. encoding/binary: benchmark old ns/op new ns/op delta BenchmarkReadSlice1000Int32s 220387 217185 -1.45% BenchmarkReadStruct 12839 12910 +0.55% BenchmarkReadInts 5692 5534 -2.78% BenchmarkWriteInts 6137 6016 -1.97% BenchmarkPutUvarint32 257 241 -6.23% BenchmarkPutUvarint64 812 754 -7.14% benchmark old MB/s new MB/s speedup BenchmarkReadSlice1000Int32s 18.15 18.42 1.01x BenchmarkReadStruct 5.45 5.42 0.99x BenchmarkReadInts 5.27 5.42 1.03x BenchmarkWriteInts 4.89 4.99 1.02x BenchmarkPutUvarint32 15.56 16.57 1.06x BenchmarkPutUvarint64 9.85 10.60 1.08x crypto/des: benchmark old ns/op new ns/op delta BenchmarkEncrypt 7002 5169 -26.18% BenchmarkDecrypt 7015 5195 -25.94% benchmark old MB/s new MB/s speedup BenchmarkEncrypt 1.14 1.55 1.36x BenchmarkDecrypt 1.14 1.54 1.35x strconv: benchmark old ns/op new ns/op delta BenchmarkAtof64Decimal 457 385 -15.75% BenchmarkAtof64Float 574 479 -16.55% BenchmarkAtof64FloatExp 1035 906 -12.46% BenchmarkAtof64Big 1793 1457 -18.74% BenchmarkAtof64RandomBits 2267 2066 -8.87% BenchmarkAtof64RandomFloats 1416 1194 -15.68% BenchmarkAtof32Decimal 451 379 -15.96% BenchmarkAtof32Float 547 435 -20.48% BenchmarkAtof32FloatExp 1095 986 -9.95% BenchmarkAtof32Random 1154 1006 -12.82% BenchmarkAtoi 1415 1380 -2.47% BenchmarkAtoiNeg 1414 1401 -0.92% BenchmarkAtoi64 1744 1671 -4.19% BenchmarkAtoi64Neg 1737 1662 -4.32% Fixes issue 1837.

Patch Set 1 #

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

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

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

Patch Set 5 : diff -r b0240b16a8e0 https://go.googlecode.com/hg/ #

Total comments: 6

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -49 lines) Patch
M src/cmd/5c/reg.c View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M src/cmd/5g/cgen.c View 1 1 chunk +10 lines, -0 lines 0 comments Download
M src/cmd/5g/ggen.c View 1 4 chunks +8 lines, -1 line 0 comments Download
M src/cmd/5g/gsubr.c View 1 2 3 11 chunks +34 lines, -15 lines 0 comments Download
M src/cmd/5g/peep.c View 1 2 3 4 5 8 chunks +76 lines, -27 lines 0 comments Download
M src/cmd/5g/reg.c View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/cmd/5l/asm.c View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/5l/optab.c View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 12
remyoudompheng
benchmark old ns/op new ns/op delta [encoding/binary] BenchmarkReadSlice1000Int32s 220387 217185 -1.45% BenchmarkReadStruct 12839 12910 +0.55% ...
11 years, 7 months ago (2013-08-04 02:07:07 UTC) #1
remyoudompheng
Hello rsc@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 7 months ago (2013-08-04 07:34:53 UTC) #2
dave_cheney.net
chromebook, some nice improvements here localhost(~/go/src/pkg/encoding/binary) % ~/go/misc/benchcmp {old,new}.txt benchmark old ns/op new ns/op delta ...
11 years, 7 months ago (2013-08-04 12:36:47 UTC) #3
rsc
LGTM
11 years, 7 months ago (2013-08-08 13:55:19 UTC) #4
remyoudompheng
On 2013/08/08 13:55:19, rsc wrote: > LGTM I wrote this when cgo was broken and ...
11 years, 7 months ago (2013-08-08 20:32:41 UTC) #5
rsc
Sure.
11 years, 7 months ago (2013-08-08 20:34:52 UTC) #6
remyoudompheng
Patch set 4 rebased on revision b0240b16a8e0 (CL 12682043)
11 years, 7 months ago (2013-08-08 21:32:18 UTC) #7
remyoudompheng
Hello rsc@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 7 months ago (2013-08-08 22:05:47 UTC) #8
remyoudompheng
all.bash seems happy now.
11 years, 7 months ago (2013-08-08 22:20:34 UTC) #9
bradfitz
LGTM On Thu, Aug 8, 2013 at 3:20 PM, <remyoudompheng@gmail.com> wrote: > all.bash seems happy ...
11 years, 7 months ago (2013-08-08 22:21:51 UTC) #10
rsc
LGTM https://codereview.appspot.com/12424043/diff/17001/src/cmd/5g/peep.c File src/cmd/5g/peep.c (right): https://codereview.appspot.com/12424043/diff/17001/src/cmd/5g/peep.c#newcode586 src/cmd/5g/peep.c:586: * MOVBS R, R' <- r s/ <- ...
11 years, 7 months ago (2013-08-08 22:57:42 UTC) #11
remyoudompheng
11 years, 7 months ago (2013-08-09 04:43:57 UTC) #12
*** Submitted as https://code.google.com/p/go/source/detail?r=76c7f1984baa ***

cmd/5c, cmd/5g, cmd/5l: turn MOVB, MOVH into plain moves, optimize short
arithmetic.

Pseudo-instructions MOVBS and MOVHS are used to clarify
the semantics of short integers vs. registers:
 * 8-bit and 16-bit values in registers are assumed to always
   be zero-extended or sign-extended depending on their type.
 * MOVB is truncation or move of an already extended value
   between registers.
 * MOVBU enforces zero-extension at the destination (register).
 * MOVBS enforces sign-extension at the destination (register).
And similarly for MOVH/MOVS/MOVHU.

The linker is adapted to assemble MOVB and MOVH to an ordinary
mov. Also a peephole pass in 5g that aims at eliminating
redundant zero/sign extensions is improved.

encoding/binary:
benchmark                              old ns/op    new ns/op    delta
BenchmarkReadSlice1000Int32s              220387       217185   -1.45%
BenchmarkReadStruct                        12839        12910   +0.55%
BenchmarkReadInts                           5692         5534   -2.78%
BenchmarkWriteInts                          6137         6016   -1.97%
BenchmarkPutUvarint32                        257          241   -6.23%
BenchmarkPutUvarint64                        812          754   -7.14%
benchmark                               old MB/s     new MB/s  speedup
BenchmarkReadSlice1000Int32s               18.15        18.42    1.01x
BenchmarkReadStruct                         5.45         5.42    0.99x
BenchmarkReadInts                           5.27         5.42    1.03x
BenchmarkWriteInts                          4.89         4.99    1.02x
BenchmarkPutUvarint32                      15.56        16.57    1.06x
BenchmarkPutUvarint64                       9.85        10.60    1.08x

crypto/des:
benchmark                              old ns/op    new ns/op    delta
BenchmarkEncrypt                            7002         5169  -26.18%
BenchmarkDecrypt                            7015         5195  -25.94%
benchmark                               old MB/s     new MB/s  speedup
BenchmarkEncrypt                            1.14         1.55    1.36x
BenchmarkDecrypt                            1.14         1.54    1.35x

strconv:
benchmark                              old ns/op    new ns/op    delta
BenchmarkAtof64Decimal                       457          385  -15.75%
BenchmarkAtof64Float                         574          479  -16.55%
BenchmarkAtof64FloatExp                     1035          906  -12.46%
BenchmarkAtof64Big                          1793         1457  -18.74%
BenchmarkAtof64RandomBits                   2267         2066   -8.87%
BenchmarkAtof64RandomFloats                 1416         1194  -15.68%
BenchmarkAtof32Decimal                       451          379  -15.96%
BenchmarkAtof32Float                         547          435  -20.48%
BenchmarkAtof32FloatExp                     1095          986   -9.95%
BenchmarkAtof32Random                       1154         1006  -12.82%
BenchmarkAtoi                               1415         1380   -2.47%
BenchmarkAtoiNeg                            1414         1401   -0.92%
BenchmarkAtoi64                             1744         1671   -4.19%
BenchmarkAtoi64Neg                          1737         1662   -4.32%

Fixes issue 1837.

R=rsc, dave, bradfitz
CC=golang-dev
https://codereview.appspot.com/12424043

https://codereview.appspot.com/12424043/diff/17001/src/cmd/5g/peep.c
File src/cmd/5g/peep.c (right):

https://codereview.appspot.com/12424043/diff/17001/src/cmd/5g/peep.c#newcode586
src/cmd/5g/peep.c:586: *   MOVBS R, R' <- r
On 2013/08/08 22:57:43, rsc wrote:
> s/ <- r//
> (confused me)

Done.

https://codereview.appspot.com/12424043/diff/17001/src/cmd/5g/peep.c#newcode594
src/cmd/5g/peep.c:594: * r is assumed to be a MOVBS/MOVBU/MOVHS/MOVHU
instruction.
On 2013/08/08 22:57:43, rsc wrote:
> MOVBS above can be MOVBS, MOVBU, MOVHS, or MOVHU.

Done.

https://codereview.appspot.com/12424043/diff/17001/src/cmd/5g/peep.c#newcode603
src/cmd/5g/peep.c:603: r1 = findpre(r, &p->from);
On 2013/08/08 22:57:43, rsc wrote:
> findpre is stronger than you need here. You don't need the check for unique
> successors. Fine to leave as is, or add a flag.

Leaving as is for the moment.
Sign in to reply to this message.

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