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

Issue 6819123: code review 6819123: cmd/gc: add division rewrite to walk pass. (Closed)

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

Description

cmd/gc: add division rewrite to walk pass. This allows 5g and 8g to benefit from the rewrite as shifts or magic multiplies. The 64-bit arithmetic is not handled there, and left in 6g. Update issue 2230.

Patch Set 1 #

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

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

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

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

Patch Set 6 : diff -r 422c00e8e022 https://go.googlecode.com/hg/ #

Patch Set 7 : diff -r 422c00e8e022 https://go.googlecode.com/hg/ #

Total comments: 8

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

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

Patch Set 10 : diff -r dc4a3f6ba179 https://go.googlecode.com/hg/ #

Patch Set 11 : diff -r dc4a3f6ba179 https://go.googlecode.com/hg/ #

Patch Set 12 : diff -r 13d81e48dc90 https://go.googlecode.com/hg/ #

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

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

Patch Set 15 : diff -r 13d81e48dc90 https://go.googlecode.com/hg/ #

Total comments: 16

Patch Set 16 : diff -r c2c17abf9eb0 https://go.googlecode.com/hg/ #

Patch Set 17 : diff -r 538f0e9733bc https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+467 lines, -189 lines) Patch
M src/cmd/5g/cgen.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -0 lines 0 comments Download
M src/cmd/5g/gg.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/5g/ggen.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +56 lines, -0 lines 0 comments Download
M src/cmd/5g/peep.c View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/6g/cgen.c View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -1 line 0 comments Download
M src/cmd/6g/gg.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M src/cmd/6g/ggen.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +58 lines, -169 lines 0 comments Download
M src/cmd/6g/peep.c View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -2 lines 0 comments Download
M src/cmd/8g/cgen.c View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M src/cmd/8g/gg.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/8g/ggen.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +36 lines, -0 lines 0 comments Download
M src/cmd/8g/gsubr.c View 1 2 3 4 5 6 7 8 9 1 chunk +16 lines, -0 lines 0 comments Download
M src/cmd/gc/walk.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 7 chunks +279 lines, -17 lines 0 comments Download

Messages

Total messages: 32
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/
6 years, 2 months ago (2012-11-11 11:02:49 UTC) #1
remyoudompheng
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 2 months ago (2012-11-11 11:20:30 UTC) #2
remyoudompheng
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 2 months ago (2012-11-11 11:24:17 UTC) #3
remyoudompheng
The latest patch adds support for the /= %= case. Results on benchamrks from CL ...
6 years, 2 months ago (2012-11-11 11:25:19 UTC) #4
dfc
Wow. On 11/11/2012, at 22:25, remyoudompheng@gmail.com wrote: > The latest patch adds support for the ...
6 years, 2 months ago (2012-11-11 12:03:33 UTC) #5
remyoudompheng
On 386 (actually a Core Quad with GOARCH=386) benchmark old ns/op new ns/op delta BenchmarkDiv1 ...
6 years, 2 months ago (2012-11-11 12:15:39 UTC) #6
remyoudompheng
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 2 months ago (2012-11-11 13:08:17 UTC) #7
remyoudompheng
Added unsigned magic multiply treatment: * 32-bit case is suboptimal because it needs a 32*32->64 ...
6 years, 2 months ago (2012-11-11 13:17:17 UTC) #8
remyoudompheng
Hello golang-dev@googlegroups.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 2 months ago (2012-11-11 13:43:07 UTC) #9
remyoudompheng
Added the signed magic multiply. Feel free to benchmark. Adding support for constants in cgen64.c ...
6 years, 2 months ago (2012-11-11 13:43:48 UTC) #10
dfc
Some minor suggestions. I don't feel qualified to review the gc changes, apart from observing ...
6 years, 2 months ago (2012-11-11 22:51:51 UTC) #11
mtj1
This is all awesome, but, don't we already know that some of these are more ...
6 years, 2 months ago (2012-11-11 23:19:39 UTC) #12
remyoudompheng
Hello golang-dev@googlegroups.com, dave@cheney.net, mtj@google.com (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 2 months ago (2012-11-12 00:38:03 UTC) #13
dfc
There may be an error with patch set #8. ./all.bash fails the second phase with ...
6 years, 2 months ago (2012-11-12 04:13:21 UTC) #14
remyoudompheng
http://codereview.appspot.com/6819123/diff/13001/src/cmd/gc/walk.c File src/cmd/gc/walk.c (right): http://codereview.appspot.com/6819123/diff/13001/src/cmd/gc/walk.c#newcode894 src/cmd/gc/walk.c:894: * on 386, rewrite float ops into l = ...
6 years, 2 months ago (2012-11-12 06:55:23 UTC) #15
remyoudompheng
Hello golang-dev@googlegroups.com, dave@cheney.net, mtj@google.com (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 2 months ago (2012-11-12 06:56:05 UTC) #16
dfc
Thank you, patch set #9 works well. linux/arm: benchmark old ns/op new ns/op delta BenchmarkBinaryTree17 ...
6 years, 2 months ago (2012-11-12 07:34:14 UTC) #17
remyoudompheng
I'm not sure why JSONDecode moves. Does it use division or is it an effect ...
6 years, 2 months ago (2012-11-13 06:51:30 UTC) #18
dfc
I'll add in some debugging and see. Also, https://codereview.appspot.com/6842045/ On Tue, Nov 13, 2012 at ...
6 years, 2 months ago (2012-11-13 06:52:41 UTC) #19
dfc
here is an additional datapoint from linux/386 220887(~/go/test/bench/go1) % ~/go/misc/benchcmp {old,new}.txt benchmark old ns/op new ...
6 years, 2 months ago (2012-11-13 21:20:24 UTC) #20
dfc
patch set 11 fails for me on linux/386 220887(~/go/src) % go run ~/go/test/ken/modconst.go u8 121 ...
6 years, 2 months ago (2012-11-15 11:37:56 UTC) #21
dfc
The previous comment notwithstanding, the 386 regression is gone in patchset 11 220887(~/go/test/bench/go1) % ~/go/misc/benchcmp ...
6 years, 2 months ago (2012-11-15 11:45:12 UTC) #22
remyoudompheng
Sorry, the latest patchset is a work in progress and it misses ARM support. It ...
6 years, 2 months ago (2012-11-15 15:45:25 UTC) #23
remyoudompheng
Hello golang-dev@googlegroups.com, dave@cheney.net, mtj@google.com (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 2 months ago (2012-11-17 00:21:54 UTC) #24
remyoudompheng
The CL now implements ARM with less instructions (using the HMUL operation). GOARCH=amd64 (Core 2 ...
6 years, 2 months ago (2012-11-17 00:27:01 UTC) #25
dfc
Wonderful. Any idea why BenchmarkUint64Mod13 regresses on arm and i386 ?
6 years, 2 months ago (2012-11-17 02:59:06 UTC) #26
dfc
gentle ping
6 years, 2 months ago (2012-11-20 23:26:57 UTC) #27
remyoudompheng
I did not take a look yet. I suppose there is one more instruction or ...
6 years, 2 months ago (2012-11-21 07:28:54 UTC) #28
dfc
Yes, LGTM with thanks. I think it is important to wait for Ian Nigel or ...
6 years, 2 months ago (2012-11-21 07:33:40 UTC) #29
rsc
LGTM Thank you for the new comments. For a future CL if you are looking ...
6 years, 1 month ago (2012-11-26 16:28:34 UTC) #30
remyoudompheng
https://codereview.appspot.com/6819123/diff/16015/src/cmd/5g/cgen.c File src/cmd/5g/cgen.c (right): https://codereview.appspot.com/6819123/diff/16015/src/cmd/5g/cgen.c#newcode266 src/cmd/5g/cgen.c:266: case OHMUL: On 2012/11/26 16:28:34, rsc wrote: > tab ...
6 years, 1 month ago (2012-11-26 21:18:55 UTC) #31
remyoudompheng
6 years, 1 month ago (2012-11-26 22:45:29 UTC) #32
*** Submitted as http://code.google.com/p/go/source/detail?r=8f9a26de2b20 ***

cmd/gc: add division rewrite to walk pass.

This allows 5g and 8g to benefit from the rewrite as shifts
or magic multiplies. The 64-bit arithmetic is not handled there,
and left in 6g.

Update issue 2230.

R=golang-dev, dave, mtj, iant, rsc
CC=golang-dev
http://codereview.appspot.com/6819123
Sign in to reply to this message.

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