FAQ
Reviewers: remyoudompheng, minux, rsc,

Message:
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://code.google.com/p/go


Description:
cmd/5g: copy MOVB peephole elimination from 5c

The original MOVB elimination code from 5g (commented out)
was too agressive.

The 5c version is less agressive, but results in
moderate improvements.

benchmark old ns/op new ns/op delta
BenchmarkBinaryTree17 52398590000 51876617000 -1.00%
BenchmarkFannkuch11 33195678000 33081147000 -0.35%
BenchmarkGobDecode 116363550 113923650 -2.10%
BenchmarkGobEncode 55769660 54918220 -1.53%
BenchmarkGzip 5464050000 5462128000 -0.04%
BenchmarkGunzip 1060395000 1058777000 -0.15%
BenchmarkJSONEncode 729699600 721032800 -1.19%
BenchmarkJSONDecode 1724457000 1702515000 -1.27%
BenchmarkMandelbrot200 45710440 45667740 -0.09%
BenchmarkParse 58709720 58497940 -0.36%
BenchmarkRevcomp 134762500 129064900 -4.23%
BenchmarkTemplate 1881347000 1831787000 -2.63%

benchmark old MB/s new MB/s speedup
BenchmarkGobDecode 6.60 6.74 1.02x
BenchmarkGobEncode 13.76 13.98 1.02x
BenchmarkGzip 3.55 3.55 1.00x
BenchmarkGunzip 18.30 18.33 1.00x
BenchmarkJSONEncode 2.66 2.69 1.01x
BenchmarkJSONDecode 1.13 1.14 1.01x
BenchmarkParse 0.99 0.99 1.00x
BenchmarkRevcomp 18.86 19.69 1.04x
BenchmarkTemplate 1.03 1.06 1.03x

Please review this at http://codereview.appspot.com/6686044/

Affected files:
M src/cmd/5g/peep.c


Index: src/cmd/5g/peep.c
===================================================================
--- a/src/cmd/5g/peep.c
+++ b/src/cmd/5g/peep.c
@@ -133,51 +133,50 @@
if(t)
goto loop1;

-return;
-
-#ifdef NOTDEF
+ /*
+ * look for MOVB x,R; MOVB R,R
+ */
for(r=firstr; r!=R; r=r->link) {
p = r->prog;
switch(p->as) {
-// case AEOR:
-// /*
-// * EOR -1,x,y => MVN x,y
-// */
-// if(isdconst(&p->from) && p->from.offset == -1) {
-// p->as = AMVN;
-// p->from.type = D_REG;
-// if(p->reg != NREG)
-// p->from.reg = p->reg;
-// else
-// p->from.reg = p->to.reg;
-// p->reg = NREG;
-// }
-// break;
-
+ default:
+ continue;
+ case AEOR:
+ /*
+ * EOR -1,x,y => MVN x,y
+ */
+ if(p->from.type == D_CONST && p->from.offset == -1) {
+ p->as = AMVN;
+ p->from.type = D_REG;
+ if(p->reg != NREG)
+ p->from.reg = p->reg;
+ else
+ p->from.reg = p->to.reg;
+ p->reg = NREG;
+ }
+ continue;
case AMOVH:
case AMOVHU:
case AMOVB:
case AMOVBU:
- /*
- * look for MOVB x,R; MOVB R,R
- */
if(p->to.type != D_REG)
- break;
- if(r1 == R)
- break;
- p1 = r1->prog;
- if(p1->as != p->as)
- break;
- if(p1->from.type != D_REG || p1->from.reg != p->to.reg)
- break;
- if(p1->to.type != D_REG || p1->to.reg != p->to.reg)
- break;
- excise(r1);
+ continue;
break;
}
r1 = r->link;
+ if(r1 == R)
+ continue;
+ p1 = r1->prog;
+ if(p1->as != p->as)
+ continue;
+ if(p1->from.type != D_REG || p1->from.reg != p->to.reg)
+ continue;
+ if(p1->to.type != D_REG || p1->to.reg != p->to.reg)
+ continue;
+ excise(r1);
}

+#ifdef NOTDEF
// for(r=firstr; r!=R; r=r->link) {
// p = r->prog;
// switch(p->as) {

Search Discussions

  • Remyoudompheng at Oct 13, 2012 at 8:01 am
    http://codereview.appspot.com/6686044/diff/1002/src/cmd/5g/peep.c
    File src/cmd/5g/peep.c (right):

    http://codereview.appspot.com/6686044/diff/1002/src/cmd/5g/peep.c#newcode138
    src/cmd/5g/peep.c:138: */
    why did this comment move? the EOR case below has nothing to do with it.

    http://codereview.appspot.com/6686044/diff/1002/src/cmd/5g/peep.c#newcode168
    src/cmd/5g/peep.c:168: continue;
    why move these lines? it seems the thing you have corrected is the
    misplaced r1 = r->link

    http://codereview.appspot.com/6686044/
  • Rémy Oudompheng at Oct 13, 2012 at 8:13 am
    Did you find an actual case where these kinds of self-moves were
    happening? It seems that for the standard case of compiling cmd/go,
    the output is exactly identical before and after the patch.

    Rémy.
  • Dave Cheney at Oct 13, 2012 at 8:26 am
    This is a direct c&p of the 5c version for consistency.
    On 13/10/2012, at 19:01, remyoudompheng@gmail.com wrote:


    http://codereview.appspot.com/6686044/diff/1002/src/cmd/5g/peep.c
    File src/cmd/5g/peep.c (right):

    http://codereview.appspot.com/6686044/diff/1002/src/cmd/5g/peep.c#newcode138
    src/cmd/5g/peep.c:138: */
    why did this comment move? the EOR case below has nothing to do with it.

    http://codereview.appspot.com/6686044/diff/1002/src/cmd/5g/peep.c#newcode168
    src/cmd/5g/peep.c:168: continue;
    why move these lines? it seems the thing you have corrected is the
    misplaced r1 = r->link

    http://codereview.appspot.com/6686044/
  • Minux at Oct 13, 2012 at 8:44 am

    On Sat, Oct 13, 2012 at 4:26 PM, Dave Cheney wrote:

    This is a direct c&p of the 5c version for consistency.
    I agree with remy that we need to keep the history of 5g/peep.c as clean as
    possible,
    so please don't move those lines.

    In fact, we probably should change 5c/peep.c as well, because the first
    comment
    block is indeed misplaced.
  • Dave Cheney at Oct 13, 2012 at 9:04 am
    No problem. I'll repurpose tomorrow.
    On 13/10/2012, at 19:44, minux wrote:

    On Sat, Oct 13, 2012 at 4:26 PM, Dave Cheney wrote:
    This is a direct c&p of the 5c version for consistency.
    I agree with remy that we need to keep the history of 5g/peep.c as clean as possible,
    so please don't move those lines.

    In fact, we probably should change 5c/peep.c as well, because the first comment
    block is indeed misplaced.
  • Dave at Oct 13, 2012 at 11:36 pm

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 13, '12 at 1:29a
activeOct 13, '12 at 11:36p
posts7
users3
websitegolang.org

3 users in discussion

Dave: 4 posts Rémy Oudompheng: 2 posts Minux: 1 post

People

Translate

site design / logo © 2021 Grokbase