code review 13602043: cmd/5g, cmd/6g, cmd/8g: simplify for loop in bitmap gen...

cmd/5g, cmd/6g, cmd/8g: simplify for loop in bitmap generation

Lucio De Re reports that the more complex
loop miscompiles on Plan 9.

[rsc, 2013-09-06 20:49:09]

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

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

[rsc, 2013-09-06 20:49:13]

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

cmd/5g, cmd/6g, cmd/8g: simplify for loop in bitmap generation

Lucio De Re reports that the more complex
loop miscompiles on Plan 9.

R=ken2
CC=golang-dev
https://codereview.appspot.com/13602043

[ken3, 2013-09-06 21:24:15]

On 2013/09/06 20:49:13, rsc wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=db7810a9efbc ***
> 
> cmd/5g, cmd/6g, cmd/8g: simplify for loop in bitmap generation
> 
> Lucio De Re reports that the more complex
> loop miscompiles on Plan 9.
> 
> R=ken2
> CC=golang-dev
> https://codereview.appspot.com/13602043

lgtm
(actually, looks the same to me)

[jnj, 2013-09-07 01:44:47]

On 2013/09/06 20:49:13, rsc wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=db7810a9efbc ***
> 
> cmd/5g, cmd/6g, cmd/8g: simplify for loop in bitmap generation
> 
> Lucio De Re reports that the more complex
> loop miscompiles on Plan 9.
> 
> R=ken2
> CC=golang-dev
> https://codereview.appspot.com/13602043

I don't know why, but while Lucio's CL 13456043 fixes the 8g build for me
(plan9/386), this CL does not.

[r, 2013-09-07 01:52:21]

A little more detail would be helpful. As it stands, your message is
no more useful than, "it's broken, please fix".

-rob

[jnj, 2013-09-07 02:31:38]

On 2013/09/07 01:52:21, r wrote:
> A little more detail would be helpful. As it stands, your message is
> no more useful than, "it's broken, please fix".
> 
> -rob

Sorry. It fails in the same manner as described here:
https://groups.google.com/forum/#!topic/comp.os.plan9/hOyZ5lH4P58 (same error
message and stack trace). Tomorrow I'll try to narrow down what the difference
is between the CLs that causing the breakage.

[lucio, 2013-09-07 05:07:13]

The problem lies, if Quanstro is right, with the computation of the
offset as the last argument of the appendp() invocation.  My
adjustment, which is a simplification, moves the computation out of
the loop.

The adjustment ought to be necessary only for the i386, which is short
of registers (again, based on Quanstro's comments).

Lucio.

On 9/7/13, Rob Pike <r@golang.org> wrote:
> A little more detail would be helpful.

[ality, 2013-09-07 10:32:26]

Lucio De Re <lucio.dere@gmail.com> once said:
> The problem lies, if Quanstro is right, with the computation of the
> offset as the last argument of the appendp() invocation.  My
> adjustment, which is a simplification, moves the computation out of
> the loop.

Erik is correct. There's a bug in 8c's 64-bit
code generation. Here's a minimal reproducer:

$ cat x.c
unsigned long long x;

int f(int);

void
test(void)
{
	int a;
	
	a = f(a-x+a);
}
% go tool 8c x.c
x.c:11 out of fixed registers
%

  Anthony

[lucio, 2013-09-07 11:42:57]

Thank you for the confirmation, Anthony.  I hope that does not inspire
the Go developers to suggest we compel the Plan 9 developers to fix
the problem on their side :-)

There is something confusing about the types involved in the loop I
tried to simplify.  There are vlongs, uints and ints variously
involved.  Very ugly under the covers and I could not quite identify
the rationale for the confusion.

Lucio,

On 9/7/13, Anthony Martin <ality@pbrane.org> wrote:
> Lucio De Re <lucio.dere@gmail.com> once said:
>> The problem lies, if Quanstro is right, with the computation of the
>> offset as the last argument of the appendp() invocation.  My
>> adjustment, which is a simplification, moves the computation out of
>> the loop.
>
> Erik is correct. There's a bug in 8c's 64-bit
> code generation. Here's a minimal reproducer:
>
> $ cat x.c
> unsigned long long x;
>
> int f(int);
>
> void
> test(void)
> {
> 	int a;
> 	
> 	a = f(a-x+a);
> }
> % go tool 8c x.c
> x.c:11 out of fixed registers
> %
>
>   Anthony
>

[mischief, 2013-09-09 11:30:05]

On 2013/09/07 11:42:57, lucio wrote:
> Thank you for the confirmation, Anthony.  I hope that does not inspire
> the Go developers to suggest we compel the Plan 9 developers to fix
> the problem on their side :-)
> 
> There is something confusing about the types involved in the loop I
> tried to simplify.  There are vlongs, uints and ints variously
> involved.  Very ugly under the covers and I could not quite identify
> the rationale for the confusion.
> 
> Lucio,
> 
> On 9/7/13, Anthony Martin <mailto:ality@pbrane.org> wrote:
> > Lucio De Re <mailto:lucio.dere@gmail.com> once said:
> >> The problem lies, if Quanstro is right, with the computation of the
> >> offset as the last argument of the appendp() invocation.  My
> >> adjustment, which is a simplification, moves the computation out of
> >> the loop.
> >
> > Erik is correct. There's a bug in 8c's 64-bit
> > code generation. Here's a minimal reproducer:
> >
> > $ cat x.c
> > unsigned long long x;
> >
> > int f(int);
> >
> > void
> > test(void)
> > {
> > 	int a;
> > 	
> > 	a = f(a-x+a);
> > }
> > % go tool 8c x.c
> > x.c:11 out of fixed registers
> > %
> >
> >   Anthony
> >

this fails in the same manner as described in the 9fans post on 9front with 8c.
using the prior cl by lucio (13456043) does work.