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

Issue 6574043: code review 6574043: cmd/8g: do not take the address of string/slice for &s[i] (Closed)

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

Description

cmd/8g: do not take the address of string/slice for &s[i] A similar change was made in 6g recently. LEALs in cmd/go: 31440 before, 27867 after. benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 7065794000 6723617000 -4.84% BenchmarkFannkuch11 7767395000 7477945000 -3.73% BenchmarkGobDecode 34708140 34857820 +0.43% BenchmarkGobEncode 10998780 10960060 -0.35% BenchmarkGzip 1603630000 1471052000 -8.27% BenchmarkGunzip 242573900 240650400 -0.79% BenchmarkJSONEncode 120842200 117966100 -2.38% BenchmarkJSONDecode 247254900 249103100 +0.75% BenchmarkMandelbrot200 29237330 29241790 +0.02% BenchmarkParse 8111320 8096865 -0.18% BenchmarkRevcomp 2595780000 2694153000 +3.79% BenchmarkTemplate 276679600 264497000 -4.40% benchmark old ns/op new ns/op delta BenchmarkAppendFloatDecimal 429 416 -3.03% BenchmarkAppendFloat 780 740 -5.13% BenchmarkAppendFloatExp 746 700 -6.17% BenchmarkAppendFloatNegExp 752 694 -7.71% BenchmarkAppendFloatBig 1228 1108 -9.77% BenchmarkAppendFloat32Integer 457 416 -8.97% BenchmarkAppendFloat32ExactFraction 662 631 -4.68% BenchmarkAppendFloat32Point 771 735 -4.67% BenchmarkAppendFloat32Exp 722 672 -6.93% BenchmarkAppendFloat32NegExp 724 659 -8.98% BenchmarkAppendFloat64Fixed1 429 400 -6.76% BenchmarkAppendFloat64Fixed2 463 442 -4.54% Update issue 1914.

Patch Set 1 #

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

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

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

Total comments: 2

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -77 lines) Patch
M src/cmd/8g/cgen.c View 1 2 3 4 5 chunks +77 lines, -76 lines 0 comments Download
M src/cmd/8g/gg.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 6
remyoudompheng
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 6 months ago (2012-09-25 06:32:22 UTC) #1
remyoudompheng
The change makes 8g very similar to 6g. The preamble was changed to make cgenindex ...
11 years, 6 months ago (2012-09-25 06:37:47 UTC) #2
remyoudompheng
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 6 months ago (2012-09-30 07:41:12 UTC) #3
DMorsing
LGTM, but I have a limited understanding of codegen, so I'd wait for Russ.
11 years, 6 months ago (2012-09-30 09:51:21 UTC) #4
rsc
LGTM https://codereview.appspot.com/6574043/diff/7001/src/cmd/8g/cgen.c File src/cmd/8g/cgen.c (right): https://codereview.appspot.com/6574043/diff/7001/src/cmd/8g/cgen.c#newcode464 src/cmd/8g/cgen.c:464: cgenindex(Node *n, Node *res) Please rename this to ...
11 years, 6 months ago (2012-10-01 21:11:20 UTC) #5
remyoudompheng
11 years, 6 months ago (2012-10-02 06:19:32 UTC) #6
*** Submitted as http://code.google.com/p/go/source/detail?r=53a8e1ce580e ***

cmd/8g: do not take the address of string/slice for &s[i]

A similar change was made in 6g recently.

LEALs in cmd/go: 31440 before, 27867 after.

benchmark                 old ns/op    new ns/op    delta
BenchmarkBinaryTree17    7065794000   6723617000   -4.84%
BenchmarkFannkuch11      7767395000   7477945000   -3.73%
BenchmarkGobDecode         34708140     34857820   +0.43%
BenchmarkGobEncode         10998780     10960060   -0.35%
BenchmarkGzip            1603630000   1471052000   -8.27%
BenchmarkGunzip           242573900    240650400   -0.79%
BenchmarkJSONEncode       120842200    117966100   -2.38%
BenchmarkJSONDecode       247254900    249103100   +0.75%
BenchmarkMandelbrot200     29237330     29241790   +0.02%
BenchmarkParse              8111320      8096865   -0.18%
BenchmarkRevcomp         2595780000   2694153000   +3.79%
BenchmarkTemplate         276679600    264497000   -4.40%

benchmark                              old ns/op    new ns/op    delta
BenchmarkAppendFloatDecimal                  429          416   -3.03%
BenchmarkAppendFloat                         780          740   -5.13%
BenchmarkAppendFloatExp                      746          700   -6.17%
BenchmarkAppendFloatNegExp                   752          694   -7.71%
BenchmarkAppendFloatBig                     1228         1108   -9.77%
BenchmarkAppendFloat32Integer                457          416   -8.97%
BenchmarkAppendFloat32ExactFraction          662          631   -4.68%
BenchmarkAppendFloat32Point                  771          735   -4.67%
BenchmarkAppendFloat32Exp                    722          672   -6.93%
BenchmarkAppendFloat32NegExp                 724          659   -8.98%
BenchmarkAppendFloat64Fixed1                 429          400   -6.76%
BenchmarkAppendFloat64Fixed2                 463          442   -4.54%

Update issue 1914.

R=golang-dev, daniel.morsing, rsc
CC=golang-dev
http://codereview.appspot.com/6574043

http://codereview.appspot.com/6574043/diff/7001/src/cmd/8g/cgen.c
File src/cmd/8g/cgen.c (right):

http://codereview.appspot.com/6574043/diff/7001/src/cmd/8g/cgen.c#newcode464
src/cmd/8g/cgen.c:464: cgenindex(Node *n, Node *res)
On 2012/10/01 21:11:20, rsc wrote:
> Please rename this to igenindex. The cgen functions write to the destination
> described by res - they don't edit the res Node itself. This is rewriting the
> res node itself, so it's more like igen now.
> 

Done.
Sign in to reply to this message.

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