code review 6249071: cmd/gc: recognize u<<1 op u>>31 as a rotate when op is ...

cmd/gc: recognize u<<1 op u>>31 as a rotate when op is ^, not just |.

[nigeltao, 2012-05-31 13:17:31]

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

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

[rsc, 2012-05-31 13:42:51]

The change is fine but it needs to be tested.
    cd $GOROOT/test; ./run
(but not all.bash) runs the output of test/rotate.go.
Please add ^ tests there.

Thanks.
Russ

[nigeltao, 2012-05-31 22:19:28]

On 31 May 2012 23:42, Russ Cox <rsc@golang.org> wrote:
> The change is fine but it needs to be tested.
>    cd $GOROOT/test; ./run
> (but not all.bash) runs the output of test/rotate.go.

On my laptop (2GB ram), running 6g on the 45KLOC output of
test/rotate.go causes so much swapping (in addition to writing a 28MB
.6 file) that my X server goes unresponsive for around 3 minutes.

Should this CL also change test/rotate.go to be less comprehensive?

[rsc, 2012-06-02 17:15:19]

On Thu, May 31, 2012 at 6:19 PM, Nigel Tao <nigeltao@golang.org> wrote:
> On my laptop (2GB ram), running 6g on the 45KLOC output of
> test/rotate.go causes so much swapping (in addition to writing a 28MB
> .6 file) that my X server goes unresponsive for around 3 minutes.
>
> Should this CL also change test/rotate.go to be less comprehensive?

Maybe, but I'd rather not.  It has caught mistakes I would not have
expected, which makes me hesitant to throwaway tests that I think
aren't going to help. Does compiling with -N help at all?

We've never bothered with 6g memory usage but if you'd like to look
into that (maybe compile the C++ google-perftools and run with it)
that'd be fine.

Russ

[nigeltao, 2012-06-03 12:01:03]

PTAL.

[rsc, 2012-06-03 14:35:07]

http://codereview.appspot.com/6249071/diff/9001/test/rotate.go
File test/rotate.go (right):

http://codereview.appspot.com/6249071/diff/9001/test/rotate.go#newcode125
test/rotate.go:125: uop, iop, opc := uor, ior, '|'
Nice.

http://codereview.appspot.com/6249071/diff/9001/test/rotate.go#newcode126
test/rotate.go:126: if rand.Intn(2) == 0 {
I had intended this to be an exhaustive test. Using rand here avoids creating
more tests but drops the coverage by half. Instead of this, can you make a new
loop above, inside the existing two:

for op := 0; op < 2; op++ {

and then here if op == 1 {

If this causes memory usage problems I'd prefer to address them separately.

[nigeltao, 2012-06-03 23:15:26]

http://codereview.appspot.com/6249071/diff/9001/test/rotate.go
File test/rotate.go (right):

http://codereview.appspot.com/6249071/diff/9001/test/rotate.go#newcode126
test/rotate.go:126: if rand.Intn(2) == 0 {
On 2012/06/03 14:35:08, rsc wrote:
> I had intended this to be an exhaustive test. Using rand here avoids creating
> more tests but drops the coverage by half. Instead of this, can you make a new
> loop above, inside the existing two:
> 
> for op := 0; op < 2; op++ {
> 
> and then here if op == 1 {
> 
> If this causes memory usage problems I'd prefer to address them separately.

Done.

[rsc, 2012-06-03 23:54:49]

LGTM

[nigeltao, 2012-06-04 10:53:53]

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

cmd/gc: recognize u<<1 op u>>31 as a rotate when op is ^, not just |.

R=rsc
CC=golang-dev
http://codereview.appspot.com/6249071