FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
cmd/6g: fix crash in cgen_bmul.

Used to print:
../test/torture.go:116: internal compiler error: bad width: 0463
(../test/torture.go:116) MOVB ,BX (0, 8)

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

Affected files:
M src/cmd/6g/ggen.c
M test/torture.go


Index: src/cmd/6g/ggen.c
===================================================================
--- a/src/cmd/6g/ggen.c
+++ b/src/cmd/6g/ggen.c
@@ -984,7 +984,7 @@
void
cgen_bmul(int op, Node *nl, Node *nr, Node *res)
{
- Node n1, n2, *tmp;
+ Node n1, n2, n1b, n2b, *tmp;
Type *t;
int a;

@@ -1000,10 +1000,18 @@
nr = tmp;
}

- regalloc(&n1, t, res);
- cgen(nl, &n1);
- regalloc(&n2, t, N);
- cgen(nr, &n2);
+ regalloc(&n1b, nl->type, res);
+ cgen(nl, &n1b);
+ regalloc(&n1, t, &n1b);
+ gmove(&n1b, &n1);
+ regfree(&n1b);
+
+ regalloc(&n2b, nr->type, N);
+ cgen(nr, &n2b);
+ regalloc(&n2, t, &n2b);
+ gmove(&n2b, &n2);
+ regfree(&n2b);
+
a = optoas(op, t);
gins(a, &n2, &n1);
regfree(&n2);
Index: test/torture.go
===================================================================
--- a/test/torture.go
+++ b/test/torture.go
@@ -58,6 +58,64 @@
m[0][3]*m[1][2]*m[2][1]*m[3][0]
}

+// Compute the determinant of a 4x4-matrix by the sum
+// over all index permutations.
+func determinantInt(m [4][4]int) int {
+ return m[0][0]*m[1][1]*m[2][2]*m[3][3] -
+ m[0][0]*m[1][1]*m[2][3]*m[3][2] -
+ m[0][0]*m[1][2]*m[2][1]*m[3][3] +
+ m[0][0]*m[1][2]*m[2][3]*m[3][1] +
+ m[0][0]*m[1][3]*m[2][1]*m[3][2] -
+ m[0][0]*m[1][3]*m[2][2]*m[3][1] -
+ m[0][1]*m[1][0]*m[2][2]*m[3][3] +
+ m[0][1]*m[1][0]*m[2][3]*m[3][2] +
+ m[0][1]*m[1][2]*m[2][0]*m[3][3] -
+ m[0][1]*m[1][2]*m[2][3]*m[3][0] -
+ m[0][1]*m[1][3]*m[2][0]*m[3][2] +
+ m[0][1]*m[1][3]*m[2][2]*m[3][0] +
+ m[0][2]*m[1][0]*m[2][1]*m[3][3] -
+ m[0][2]*m[1][0]*m[2][3]*m[3][1] -
+ m[0][2]*m[1][1]*m[2][0]*m[3][3] +
+ m[0][2]*m[1][1]*m[2][3]*m[3][0] +
+ m[0][2]*m[1][3]*m[2][0]*m[3][1] -
+ m[0][2]*m[1][3]*m[2][1]*m[3][0] -
+ m[0][3]*m[1][0]*m[2][1]*m[3][2] +
+ m[0][3]*m[1][0]*m[2][2]*m[3][1] +
+ m[0][3]*m[1][1]*m[2][0]*m[3][2] -
+ m[0][3]*m[1][1]*m[2][2]*m[3][0] -
+ m[0][3]*m[1][2]*m[2][0]*m[3][1] +
+ m[0][3]*m[1][2]*m[2][1]*m[3][0]
+}
+
+// Compute the determinant of a 4x4-matrix by the sum
+// over all index permutations.
+func determinantByte(m [4][4]byte) byte {
+ return m[0][0]*m[1][1]*m[2][2]*m[3][3] -
+ m[0][0]*m[1][1]*m[2][3]*m[3][2] -
+ m[0][0]*m[1][2]*m[2][1]*m[3][3] +
+ m[0][0]*m[1][2]*m[2][3]*m[3][1] +
+ m[0][0]*m[1][3]*m[2][1]*m[3][2] -
+ m[0][0]*m[1][3]*m[2][2]*m[3][1] -
+ m[0][1]*m[1][0]*m[2][2]*m[3][3] +
+ m[0][1]*m[1][0]*m[2][3]*m[3][2] +
+ m[0][1]*m[1][2]*m[2][0]*m[3][3] -
+ m[0][1]*m[1][2]*m[2][3]*m[3][0] -
+ m[0][1]*m[1][3]*m[2][0]*m[3][2] +
+ m[0][1]*m[1][3]*m[2][2]*m[3][0] +
+ m[0][2]*m[1][0]*m[2][1]*m[3][3] -
+ m[0][2]*m[1][0]*m[2][3]*m[3][1] -
+ m[0][2]*m[1][1]*m[2][0]*m[3][3] +
+ m[0][2]*m[1][1]*m[2][3]*m[3][0] +
+ m[0][2]*m[1][3]*m[2][0]*m[3][1] -
+ m[0][2]*m[1][3]*m[2][1]*m[3][0] -
+ m[0][3]*m[1][0]*m[2][1]*m[3][2] +
+ m[0][3]*m[1][0]*m[2][2]*m[3][1] +
+ m[0][3]*m[1][1]*m[2][0]*m[3][2] -
+ m[0][3]*m[1][1]*m[2][2]*m[3][0] -
+ m[0][3]*m[1][2]*m[2][0]*m[3][1] +
+ m[0][3]*m[1][2]*m[2][1]*m[3][0]
+}
+
// A right-leaning tree of byte multiplications.
func righttree(a, b, c, d uint8) uint8 {
return a * (b * (c * (d *

Search Discussions

  • Nigeltao at Oct 23, 2012 at 8:03 am
    Does the codegen change for something less torturous, like:
    var w, x, y, z byte
    return w*x*y*z

    https://codereview.appspot.com/6736068/
  • Remyoudompheng at Oct 23, 2012 at 8:49 pm

    On 2012/10/23 07:55:23, nigeltao wrote:
    Does the codegen change for something less torturous, like:
    var w, x, y, z byte
    return w*x*y*z
    It makes some unnecessary zero-extensions which are not caught by the
    peephole optimizer.

    For
    func G(a, b, c, d byte) byte {
    return a*b*c*d
    }

    Before:
    --- prog list "G" ---
    0000 (/tmp/byte.go:7) TEXT G+0(SB),$0-16
    0001 (/tmp/byte.go:8) MOVBQZX a+0(FP),BX
    0002 (/tmp/byte.go:8) MOVBQZX b+1(FP),BP
    0003 (/tmp/byte.go:8) IMULQ BP,BX
    0004 (/tmp/byte.go:8) MOVBQZX c+2(FP),BP
    0005 (/tmp/byte.go:8) IMULQ BP,BX
    0006 (/tmp/byte.go:8) MOVBQZX d+3(FP),BP
    0007 (/tmp/byte.go:8) IMULQ BP,BX
    0008 (/tmp/byte.go:8) MOVB BX,.noname+8(FP)
    0009 (/tmp/byte.go:8) RET ,

    After:
    --- prog list "G" ---
    0000 (/tmp/byte.go:7) TEXT G+0(SB),$0-16
    0001 (/tmp/byte.go:8) MOVB a+0(FP),BX
    0002 (/tmp/byte.go:8) MOVBQZX BX,BX
    0003 (/tmp/byte.go:8) MOVB b+1(FP),BP
    0004 (/tmp/byte.go:8) MOVBQZX BP,BP
    0005 (/tmp/byte.go:8) IMULQ BP,BX
    0006 (/tmp/byte.go:8) MOVBQZX BX,BX
    0007 (/tmp/byte.go:8) MOVB c+2(FP),BP
    0008 (/tmp/byte.go:8) MOVBQZX BP,BP
    0009 (/tmp/byte.go:8) IMULQ BP,BX
    0010 (/tmp/byte.go:8) MOVBQZX BX,BX
    0011 (/tmp/byte.go:8) MOVB d+3(FP),BP
    0012 (/tmp/byte.go:8) MOVBQZX BP,BP
    0013 (/tmp/byte.go:8) IMULQ BP,BX
    0014 (/tmp/byte.go:8) MOVB BX,.noname+8(FP)
    0015 (/tmp/byte.go:8) RET ,
    0016 (/tmp/byte.go:9) CALL ,runtime.throwreturn+0(SB)
    0017 (/tmp/byte.go:9) RET ,

    But this patch is essentially reverting a simplification that I think I
    submitted some weeks ago. I am probably working around another bug or
    unplanned use of gins, so I could keep the current cgen_bmul and instead
    fix the bug.

    http://codereview.appspot.com/6736068/
  • Russ Cox at Oct 24, 2012 at 7:12 pm
    Does the latest CL still use MOVB? That's very expensive.
  • Remyoudompheng at Oct 23, 2012 at 10:17 pm
    Hello golang-dev@googlegroups.com, nigeltao@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6736068/
  • Remyoudompheng at Oct 23, 2012 at 10:28 pm
    Fixing things was too hard. cgen simply doesn't expect operands of
    different types. Or well, it can accept them but not in all cases. When
    going through the sudoaddable branch things get screwed.

    If sudoaddable filled a Node instead of an Addr (while still keeping the
    same logic), it could use gmove instead of gins, that would make things
    a bit easier, and maybe avoid patching partially generated instructions.

    I have simply adapted cgen_bmul to work around that and keep the same
    generated code.

    http://codereview.appspot.com/6736068/
  • Nigeltao at Oct 24, 2012 at 12:04 pm
  • Remyoudompheng at Oct 24, 2012 at 7:59 pm
    It doesn't, but that's because it is eliminated at peephole-time.

    Here is where we stand:

    $ cat /tmp/byte.go
    package b

    func F(s [4]byte) byte {
    return s[0]*s[1] + s[2]*s[3]
    }

    func F2(s [4]byte, i, j int) byte {
    return s[i]*s[i] + s[j]*s[j]
    }

    func G(a, b, c, d byte) byte {
    return a*b*c*d
    }
    $ bin/go tool 6g -S /tmp/byte.go

    --- prog list "F" ---
    0000 (/tmp/byte.go:3) TEXT F+0(SB),$0-16
    0001 (/tmp/byte.go:4) MOVBQZX s+0(FP),BX
    0002 (/tmp/byte.go:4) MOVBQZX s+1(FP),BP
    0003 (/tmp/byte.go:4) IMULQ BP,BX
    0004 (/tmp/byte.go:4) MOVBQZX s+2(FP),BP
    0005 (/tmp/byte.go:4) MOVBQZX s+3(FP),R8
    0006 (/tmp/byte.go:4) IMULQ R8,BP
    0007 (/tmp/byte.go:4) ADDQ BP,BX
    0008 (/tmp/byte.go:4) MOVB BX,.noname+8(FP)
    0009 (/tmp/byte.go:4) RET ,

    --- prog list "F2" ---
    0010 (/tmp/byte.go:7) TEXT F2+0(SB),$0-32
    0011 (/tmp/byte.go:7) MOVQ i+8(FP),BP
    0012 (/tmp/byte.go:7) MOVQ j+16(FP),AX
    0013 (/tmp/byte.go:8) MOVQ BP,CX
    0014 (/tmp/byte.go:8) LEAQ s+0(FP),R8
    0015 (/tmp/byte.go:8) CMPQ BP,$4
    0016 (/tmp/byte.go:8) JCS $1,19
    0017 (/tmp/byte.go:8) CALL ,runtime.panicindex+0(SB)
    0018 (/tmp/byte.go:8) UNDEF ,
    0019 (/tmp/byte.go:8) MOVBQZX (R8)(BP*1),BX
    0020 (/tmp/byte.go:8) MOVQ CX,R8
    0021 (/tmp/byte.go:8) LEAQ s+0(FP),R9
    0022 (/tmp/byte.go:8) CMPQ CX,$4
    0023 (/tmp/byte.go:8) JCS $1,26
    0024 (/tmp/byte.go:8) CALL ,runtime.panicindex+0(SB)
    0025 (/tmp/byte.go:8) UNDEF ,
    0026 (/tmp/byte.go:8) MOVBQZX (R9)(R8*1),BP
    0027 (/tmp/byte.go:8) IMULQ BP,BX
    0028 (/tmp/byte.go:8) MOVQ AX,R8
    0029 (/tmp/byte.go:8) LEAQ s+0(FP),R9
    0030 (/tmp/byte.go:8) CMPQ AX,$4
    0031 (/tmp/byte.go:8) JCS $1,34
    0032 (/tmp/byte.go:8) CALL ,runtime.panicindex+0(SB)
    0033 (/tmp/byte.go:8) UNDEF ,
    0034 (/tmp/byte.go:8) MOVBQZX (R9)(R8*1),BP
    0035 (/tmp/byte.go:8) MOVQ AX,R9
    0036 (/tmp/byte.go:8) LEAQ s+0(FP),R10
    0037 (/tmp/byte.go:8) CMPQ AX,$4
    0038 (/tmp/byte.go:8) JCS $1,41
    0039 (/tmp/byte.go:8) CALL ,runtime.panicindex+0(SB)
    0040 (/tmp/byte.go:8) UNDEF ,
    0041 (/tmp/byte.go:8) MOVBQZX (R10)(R9*1),R8
    0042 (/tmp/byte.go:8) IMULQ R8,BP
    0043 (/tmp/byte.go:8) ADDQ BP,BX
    0044 (/tmp/byte.go:8) MOVB BX,.noname+24(FP)
    0045 (/tmp/byte.go:8) RET ,

    --- prog list "G" ---
    0046 (/tmp/byte.go:11) TEXT G+0(SB),$0-16
    0047 (/tmp/byte.go:12) MOVBQZX a+0(FP),BX
    0048 (/tmp/byte.go:12) MOVBQZX b+1(FP),BP
    0049 (/tmp/byte.go:12) IMULQ BP,BX
    0050 (/tmp/byte.go:12) MOVBQZX c+2(FP),BP
    0051 (/tmp/byte.go:12) IMULQ BP,BX
    0052 (/tmp/byte.go:12) MOVBQZX d+3(FP),BP
    0053 (/tmp/byte.go:12) IMULQ BP,BX
    0054 (/tmp/byte.go:12) MOVB BX,.noname+8(FP)
    0055 (/tmp/byte.go:12) RET ,


    https://codereview.appspot.com/6736068/
  • Remyoudompheng at Oct 25, 2012 at 8:56 pm
    Some users are annoyed by the bug. May I submit with Nigel's LGTM and
    since rsc's remark seems addressed, or are there other remarks?

    http://codereview.appspot.com/6736068/
  • Rsc at Oct 25, 2012 at 9:43 pm

    On 2012/10/25 20:56:48, remyoudompheng wrote:
    Some users are annoyed by the bug. May I submit with Nigel's LGTM and since
    rsc's remark seems addressed, or are there other remarks?
    yes


    https://codereview.appspot.com/6736068/
  • Remyoudompheng at Oct 25, 2012 at 10:29 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=d81bcf447d65 ***

    cmd/6g: fix crash in cgen_bmul.

    Used to print:
    ../test/torture.go:116: internal compiler error: bad width: 0463
    (../test/torture.go:116) MOVB ,BX (0, 8)

    R=nigeltao, rsc
    CC=golang-dev
    http://codereview.appspot.com/6736068


    http://codereview.appspot.com/6736068/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 22, '12 at 11:03p
activeOct 25, '12 at 10:29p
posts11
users3
websitegolang.org

3 users in discussion

Remyoudompheng: 7 posts Nigeltao: 2 posts Rsc: 2 posts

People

Translate

site design / logo © 2022 Grokbase