code review 62730043: cmd/5g: fix regopt bug in copyprop

cmd/5g: fix regopt bug in copyprop

copyau1 was assuming that it could deduce the type of the
middle register p->reg from the type of the left or right
argument: in CMPF F1, F2, the p->reg==2 must be a D_FREG
because p->from is F1, and in CMP R1, R2, the p->reg==2 must
be a D_REG because p->from is R1.

This heuristic fails for CMP $0, R2, which was causing copyau1
not to recognize p->reg==2 as a reference to R2, which was
keeping it from properly renaming the register use when
substituting registers.

cmd/5c has the right approach: look at the opcode p->as to
decide the kind of register. It is unclear where 5g's copyau1
came from; perhaps it was an attempt to avoid expanding 5c's
a2type to include new instructions used only by 5g.

Copy a2type from cmd/5c, expand to include additional instructions,
and make it crash the compiler if asked about an instruction
it does not understand (avoid silent bugs in the future if new
instructions are added).

Should fix current arm build breakage.

While we're here, fix the print statements dumping the pred and
succ info in the asm listing to pass an int arg to %.4ud
(Prog.pc is a vlong now, due to the liblink merge).

[rsc, 2014-02-13 03:54:53]

Hello ken2 (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[rsc, 2014-02-13 03:55:03]

*** Submitted as https://code.google.com/p/go/source/detail?r=6d069d383628 ***

cmd/5g: fix regopt bug in copyprop

copyau1 was assuming that it could deduce the type of the
middle register p->reg from the type of the left or right
argument: in CMPF F1, F2, the p->reg==2 must be a D_FREG
because p->from is F1, and in CMP R1, R2, the p->reg==2 must
be a D_REG because p->from is R1.

This heuristic fails for CMP $0, R2, which was causing copyau1
not to recognize p->reg==2 as a reference to R2, which was
keeping it from properly renaming the register use when
substituting registers.

cmd/5c has the right approach: look at the opcode p->as to
decide the kind of register. It is unclear where 5g's copyau1
came from; perhaps it was an attempt to avoid expanding 5c's
a2type to include new instructions used only by 5g.

Copy a2type from cmd/5c, expand to include additional instructions,
and make it crash the compiler if asked about an instruction
it does not understand (avoid silent bugs in the future if new
instructions are added).

Should fix current arm build breakage.

While we're here, fix the print statements dumping the pred and
succ info in the asm listing to pass an int arg to %.4ud
(Prog.pc is a vlong now, due to the liblink merge).

TBR=ken2
CC=golang-codereviews
https://codereview.appspot.com/62730043

[dave_cheney.net, 2014-02-13 03:55:34]

Thanks Russ


On Thu, Feb 13, 2014 at 2:55 PM, <rsc@golang.org> wrote:

> *** Submitted as
> https://code.google.com/p/go/source/detail?r=6d069d383628 ***
>
>
> cmd/5g: fix regopt bug in copyprop
>
> copyau1 was assuming that it could deduce the type of the
> middle register p->reg from the type of the left or right
> argument: in CMPF F1, F2, the p->reg==2 must be a D_FREG
> because p->from is F1, and in CMP R1, R2, the p->reg==2 must
> be a D_REG because p->from is R1.
>
> This heuristic fails for CMP $0, R2, which was causing copyau1
> not to recognize p->reg==2 as a reference to R2, which was
> keeping it from properly renaming the register use when
> substituting registers.
>
> cmd/5c has the right approach: look at the opcode p->as to
> decide the kind of register. It is unclear where 5g's copyau1
> came from; perhaps it was an attempt to avoid expanding 5c's
> a2type to include new instructions used only by 5g.
>
> Copy a2type from cmd/5c, expand to include additional instructions,
> and make it crash the compiler if asked about an instruction
> it does not understand (avoid silent bugs in the future if new
> instructions are added).
>
> Should fix current arm build breakage.
>
> While we're here, fix the print statements dumping the pred and
> succ info in the asm listing to pass an int arg to %.4ud
> (Prog.pc is a vlong now, due to the liblink merge).
>
> TBR=ken2
> CC=golang-codereviews
> https://codereview.appspot.com/62730043
>
>
> https://codereview.appspot.com/62730043/
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[ken3, 2014-02-14 09:57:43]

On 2014/02/13 03:55:34, dfc wrote:
> Thanks Russ
> 
> 
> On Thu, Feb 13, 2014 at 2:55 PM, <mailto:rsc@golang.org> wrote:
> 
> > *** Submitted as
> > https://code.google.com/p/go/source/detail?r=6d069d383628 ***
> >
> >
> > cmd/5g: fix regopt bug in copyprop
> >
> > copyau1 was assuming that it could deduce the type of the
> > middle register p->reg from the type of the left or right
> > argument: in CMPF F1, F2, the p->reg==2 must be a D_FREG
> > because p->from is F1, and in CMP R1, R2, the p->reg==2 must
> > be a D_REG because p->from is R1.
> >
> > This heuristic fails for CMP $0, R2, which was causing copyau1
> > not to recognize p->reg==2 as a reference to R2, which was
> > keeping it from properly renaming the register use when
> > substituting registers.
> >
> > cmd/5c has the right approach: look at the opcode p->as to
> > decide the kind of register. It is unclear where 5g's copyau1
> > came from; perhaps it was an attempt to avoid expanding 5c's
> > a2type to include new instructions used only by 5g.
> >
> > Copy a2type from cmd/5c, expand to include additional instructions,
> > and make it crash the compiler if asked about an instruction
> > it does not understand (avoid silent bugs in the future if new
> > instructions are added).
> >
> > Should fix current arm build breakage.
> >
> > While we're here, fix the print statements dumping the pred and
> > succ info in the asm listing to pass an int arg to %.4ud
> > (Prog.pc is a vlong now, due to the liblink merge).
> >
> > TBR=ken2
> > CC=golang-codereviews
> > https://codereview.appspot.com/62730043
> >
> >
> > https://codereview.appspot.com/62730043/
> >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >

LGTM