code review 84690044: cmd/6g: relax constraint on variables that need zeroing.

cmd/6g: relax constraint on variables that need zeroing.

On amd64p32 pointers are 32-bit-aligned and cannot be assumed to
have an offset multiple of widthreg. Instead check that they are
withptr-aligned.

Also change the threshold for region merging to 2*widthreg
instead of 2*widthptr because performance on amd64 and amd64p32
is expected to be the same.

Fixes issue 7712.

[remyoudompheng, 2014-04-05 08:33:32]

Hello rsc@golang.org, dave@cheney.net, khr@golang.org, brad@fitzpat.com (cc:
golang-codereviews@googlegroups.com),

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

[khr1, 2014-04-05 17:13:04]

LGTM.

But if widthptr < widthreg, then zerorange will have to be modified - it
zeros only multiples of widthreg, and it might now be passed a range with
an extra widthreg/2 at the end.


On Sat, Apr 5, 2014 at 1:33 AM, <remyoudompheng@gmail.com> wrote:

> Reviewers: rsc, dfc, khr, brad,
>
> Message:
> Hello rsc@golang.org, dave@cheney.net, khr@golang.org, brad@fitzpat.com
> (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cmd/6g: relax constraint on variables that need zeroing.
>
> On amd64p32 pointers are 32-bit-aligned and cannot be assumed to
> have an offset multiple of widthreg. Instead check that they are
> withptr-aligned.
>
> Also change the threshold for region merging to 2*widthreg
> instead of 2*widthptr because performance on amd64 and amd64p32
> is expected to be the same.
>
> Fixes issue 7712.
>
> Please review this at https://codereview.appspot.com/84690044/
>
> Affected files (+3, -3 lines):
>   M src/cmd/6g/ggen.c
>
>
> Index: src/cmd/6g/ggen.c
> ===================================================================
> --- a/src/cmd/6g/ggen.c
> +++ b/src/cmd/6g/ggen.c
> @@ -42,12 +42,12 @@
>                         continue;
>                 if(n->class != PAUTO)
>                         fatal("needzero class %d", n->class);
> -               if(n->type->width % widthreg != 0 || n->xoffset % widthreg
> != 0 || n->type->width == 0)
> +               if(n->type->width % widthptr != 0 || n->xoffset % widthptr
> != 0 || n->type->width == 0)
>                         fatal("var %lN has size %d offset %d", n,
> (int)n->type->width, (int)n->xoffset);
>
> -               if(lo != hi && n->xoffset + n->type->width >= lo -
> 2*widthptr) {
> +               if(lo != hi && n->xoffset + n->type->width >= lo -
> 2*widthreg) {
>                         // merge with range we already have
> -                       lo = n->xoffset;
> +                       lo = rnd(n->xoffset, widthreg);
>                         continue;
>                 }
>                 // zero old range
>
>
>

[bradfitz, 2014-04-09 18:04:15]

ping. Remy?



On Sat, Apr 5, 2014 at 10:13 AM, Keith Randall <khr@google.com> wrote:

> LGTM.
>
> But if widthptr < widthreg, then zerorange will have to be modified - it
> zeros only multiples of widthreg, and it might now be passed a range with
> an extra widthreg/2 at the end.
>
>
> On Sat, Apr 5, 2014 at 1:33 AM, <remyoudompheng@gmail.com> wrote:
>
>> Reviewers: rsc, dfc, khr, brad,
>>
>> Message:
>> Hello rsc@golang.org, dave@cheney.net, khr@golang.org, brad@fitzpat.com
>> (cc: golang-codereviews@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> cmd/6g: relax constraint on variables that need zeroing.
>>
>> On amd64p32 pointers are 32-bit-aligned and cannot be assumed to
>> have an offset multiple of widthreg. Instead check that they are
>> withptr-aligned.
>>
>> Also change the threshold for region merging to 2*widthreg
>> instead of 2*widthptr because performance on amd64 and amd64p32
>> is expected to be the same.
>>
>> Fixes issue 7712.
>>
>> Please review this at https://codereview.appspot.com/84690044/
>>
>> Affected files (+3, -3 lines):
>>   M src/cmd/6g/ggen.c
>>
>>
>> Index: src/cmd/6g/ggen.c
>> ===================================================================
>> --- a/src/cmd/6g/ggen.c
>> +++ b/src/cmd/6g/ggen.c
>> @@ -42,12 +42,12 @@
>>                         continue;
>>                 if(n->class != PAUTO)
>>                         fatal("needzero class %d", n->class);
>> -               if(n->type->width % widthreg != 0 || n->xoffset %
>> widthreg != 0 || n->type->width == 0)
>> +               if(n->type->width % widthptr != 0 || n->xoffset %
>> widthptr != 0 || n->type->width == 0)
>>                         fatal("var %lN has size %d offset %d", n,
>> (int)n->type->width, (int)n->xoffset);
>>
>> -               if(lo != hi && n->xoffset + n->type->width >= lo -
>> 2*widthptr) {
>> +               if(lo != hi && n->xoffset + n->type->width >= lo -
>> 2*widthreg) {
>>                         // merge with range we already have
>> -                       lo = n->xoffset;
>> +                       lo = rnd(n->xoffset, widthreg);
>>                         continue;
>>                 }
>>                 // zero old range
>>
>>
>>
>  --
> 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/d/optout.
>

[remyoudompheng, 2014-04-09 19:23:41]

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

cmd/6g: relax constraint on variables that need zeroing.

On amd64p32 pointers are 32-bit-aligned and cannot be assumed to
have an offset multiple of widthreg. Instead check that they are
withptr-aligned.

Also change the threshold for region merging to 2*widthreg
instead of 2*widthptr because performance on amd64 and amd64p32
is expected to be the same.

Fixes issue 7712.

LGTM=khr
R=rsc, dave, khr, brad, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/84690044