DoubleToIStub can't use ip on armv6, because the ubfx impl will clobber it

DoubleToIStub can't use ip on armv6, because the ubfx impl will clobber it

This previous change broke DoubleToIStub on armv6:
https://code.google.com/p/v8/source/detail?r=16322

The problem is that DoubleToIStub::Generate assumed that it could safely use the ip register, but on armv6 the ubfx implementation will clobber any previous value stored there.  So instead, pick another register.

Test case:
for (var i=0; i<2; i++) {
	v = 4294967295;
	v &= -2;
	print(v)
	}

This should print -2 twice, but on armv6 without this patch, it prints -2 followed by 2046.

This problem causes sunspider's bitops-nsieve-bit, crypto-md5 and crypto-sha1 tests to generate incorrect results (but the results are not checked for validity in sunspider-1.0 as available in chromium, but are checked and reported as incorrect in sunspider-1.0.2).

Thanks to Tomasz Kilarski for helping out with this.

R=bmeurer@chromium.org, rmcilroy@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=18688

[Mostyn Bramley-Moore, 2014-01-13 14:36:42]

@Benedikt, rmcilroy: PTAL

[Mostyn Bramley-Moore, 2014-01-13 16:02:22]

Hmm, I'm getting a segfault in a the larger testcase now (bitops-nsieve-bits),
I'm not sure what's wrong yet (this seemed to work previously) :/

[Mostyn Bramley-Moore, 2014-01-13 23:23:29]

Pushing and Popping in two steps doesn't segfault.  I'm not sure why this works-
are there some constraints on the order of the arguments that I haven't
realised?

[Benedikt Meurer, 2014-01-14 10:01:58]

Hm, I'm not sure. Rodolph can you shed some light on this?

[Rodolph Perfetta, 2014-01-14 11:46:19]

On 2014/01/14 10:01:58, Benedikt Meurer wrote:
> Hm, I'm not sure. Rodolph can you shed some light on this?

there is a bug in the marco assembler: the Pop three registers function at line
392 will str instead of ldr the last register. This bug only show if the three
registers are not ordered ...

[Rodolph Perfetta, 2014-01-14 11:47:27]

On 2014/01/14 11:46:19, Rodolph Perfetta wrote:
> On 2014/01/14 10:01:58, Benedikt Meurer wrote:
> > Hm, I'm not sure. Rodolph can you shed some light on this?
> 
> there is a bug in the marco assembler: the Pop three registers function at
line
> 392 will str instead of ldr the last register. This bug only show if the three
> registers are not ordered ...

I should have said the offending function is in macro-assembler-arm.h

[Benedikt Meurer, 2014-01-14 12:47:48]

On 2014/01/14 11:47:27, Rodolph Perfetta wrote:
> On 2014/01/14 11:46:19, Rodolph Perfetta wrote:
> > On 2014/01/14 10:01:58, Benedikt Meurer wrote:
> > > Hm, I'm not sure. Rodolph can you shed some light on this?
> > 
> > there is a bug in the marco assembler: the Pop three registers function at
> line
> > 392 will str instead of ldr the last register. This bug only show if the
three
> > registers are not ordered ...
> 
> I should have said the offending function is in macro-assembler-arm.h

Fixed, thanks for spotting.

[rmcilroy, 2014-01-14 14:23:30]

https://codereview.chromium.org/131823004/diff/50001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/131823004/diff/50001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:619: __ Push(scratch_high, scratch_low);
Since Benedikt has fixed the Push/Pop typo, please change this back to a single
Push, but order the registers so that it can be done in a single instruction -
e.g., Push(scratch_high, scratch_low, scratch)

[Mostyn Bramley-Moore, 2014-01-14 14:30:57]

https://codereview.chromium.org/131823004/diff/50001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/131823004/diff/50001/src/arm/code-stubs-arm.c...
src/arm/code-stubs-arm.cc:619: __ Push(scratch_high, scratch_low);
On 2014/01/14 14:23:30, rmcilroy wrote:
> Since Benedikt has fixed the Push/Pop typo, please change this back to a
single
> Push, but order the registers so that it can be done in a single instruction -
> e.g., Push(scratch_high, scratch_low, scratch)

Done.

I have verified that this works on master now.

[rmcilroy, 2014-01-14 14:50:19]

https://codereview.chromium.org/131823004/diff/140001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/131823004/diff/140001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:618: __ Push(scratch, scratch_high, scratch_low);
Please move 'scratch' to the end of the arguments i.e.:
  __ Push(scratch_high, scratch_low, scratch);
otherwise this push can't happen in a single "stm" instruction. (same with Pop
below).

[Mostyn Bramley-Moore, 2014-01-14 15:01:50]

https://codereview.chromium.org/131823004/diff/140001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/131823004/diff/140001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:618: __ Push(scratch, scratch_high, scratch_low);
On 2014/01/14 14:50:20, rmcilroy wrote:
> Please move 'scratch' to the end of the arguments i.e.:
>   __ Push(scratch_high, scratch_low, scratch);
> otherwise this push can't happen in a single "stm" instruction. (same with Pop
> below).

Done.

[Benedikt Meurer, 2014-01-15 05:31:45]

LGTM

[Benedikt Meurer, 2014-01-15 05:39:07]

Ups, NOT LGTM, latest patch causes check failures in simulator.

[Mostyn Bramley-Moore, 2014-01-15 11:07:53]

On 2014/01/15 05:39:07, Benedikt Meurer wrote:
> Ups, NOT LGTM, latest patch causes check failures in simulator.

I can see failures when doing "make arm.check arm_version=6" before this patch
(which I can investigate separately), but not in the arm_version=7 build (so
far).  I'll do a comparison to see if this patch introduces any new test
failures (it's taking a while to run on my machine).

[Benedikt Meurer, 2014-01-15 11:29:45]

I applied your patch and run make arm.optdebug.check on my Intel workstation.

[Mostyn Bramley-Moore, 2014-01-15 12:44:16]

On 2014/01/15 11:29:45, Benedikt Meurer wrote:
> I applied your patch and run make arm.optdebug.check on my Intel workstation.

I can see the errors now, looking into it.

BTW is it possible to run these tests in a way that produces better logs?  If I
just redirect the output to a file it's full of ^M's and is very hard to read.

[Jakob Kummerow, 2014-01-15 13:50:41]

On 2014/01/15 12:44:16, Mostyn Bramley-Moore wrote:
> BTW is it possible to run these tests in a way that produces better logs?  If
I
> just redirect the output to a file it's full of ^M's and is very hard to read.

tools/run-tests.py has a --progress parameter that chooses the progress
indicator. Maybe "verbose" suits you better?

If you have special requirements, it's also not very hard to add another
progress indicator. See tools/testrunner/local/progress.py.

FWIW, I personally find the default output quite convenient, as it prints plenty
of details for failed tests while being rather silent if everything goes well.

[Mostyn Bramley-Moore, 2014-01-17 12:02:10]

@Benedikt: patchset 5 seems to fix the test failures for me, apart from a
timeout sometimes (I don't think it's related though).  Note that you might need
to update to a recent master first, I had problems earlier but this patch on top
of a more recent master seems to work (I'm currently testing with git commit
ef962d53e988432b4fb6b85db3b3d3786c643bcb).

On 2014/01/15 13:50:41, Jakob wrote:
> On 2014/01/15 12:44:16, Mostyn Bramley-Moore wrote:
> > BTW is it possible to run these tests in a way that produces better logs? 
If
> I
> > just redirect the output to a file it's full of ^M's and is very hard to
read.
> 
> tools/run-tests.py has a --progress parameter that chooses the progress
> indicator. Maybe "verbose" suits you better?
> 
> If you have special requirements, it's also not very hard to add another
> progress indicator. See tools/testrunner/local/progress.py.
> 
> FWIW, I personally find the default output quite convenient, as it prints
plenty
> of details for failed tests while being rather silent if everything goes well.

Thanks- I'm going to setup some local armv6 continuous build+tests and this
looks like it might work for me.

[rmcilroy, 2014-01-17 12:09:03]

lgtm

[Benedikt Meurer, 2014-01-17 19:19:10]

LGTM. I'll land it next week unless Ross beats me. :-)

[rmcilroy, 2014-01-20 11:30:55]

Committed patchset #5 manually as r18688 (presubmit successful).

[rmcilroy, 2014-01-20 11:36:50]

On 2014/01/20 11:30:55, rmcilroy wrote:
> Committed patchset #5 manually as r18688 (presubmit successful).

I just landed this.  Thanks for the fix Mostyn!