Implement type recording for ToBoolean on ARM.

Implement type recording for ToBoolean on ARM.
Committed: http://code.google.com/p/v8/source/detail?r=8859

[Erik Corry, 2011-08-05 12:50:46]

LGTM

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1613: const Register map = r9.is(tos_) ? r7 : r9;
If we have ever seen an internal object you could start with a Smi check and an
internal object check here.  Then you could remove the special handling for
internal object on the other 10 cases.  It would be slightly slower for the
internal object case, but that is rare and arguably broken anyway.  It would
simplify things a lot.

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1615: // undefined -> false
Missing full stops on the end of comments several places.  Not your fault, but
we might as well fix it now that we are looking at the code.

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1638: // Everything with a map could be undetectable,
so check this now.
Pretty sure that it is just strings and JSObjects.  The tests will catch it if I
am wrong, at least they will if they run everything twice, one to patch the
ToBooleanStub and the second time to run it.

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1689: // internal objects -> true
Caps and full stop.

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-arm.cc#newcode1044
src/arm/lithium-arm.cc:1044: // environment then.
Do we hit this case in the tests?

I would guess it is so rare that the best way may be not to special case it.

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1589: // We've seen undefined for the first time
-> deopt.
We should just bail out if we have seen an internal object.

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1594: if
(expected.Contains(ToBooleanStub::BOOLEAN)) {
This 'if' and the next can be merged.

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1601: __ LoadRoot(ip, Heap::kTrueValueRootIndex);
We have CompareRoot in the macro assembler.

[Sven Panne, 2011-08-09 07:58:21]

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1613: const Register map = r9.is(tos_) ? r7 : r9;
On 2011/08/05 12:50:46, Erik Corry wrote:
> If we have ever seen an internal object you could start with a Smi check ...

I think we can get rid of all internal object checks when we change
apinatives.js slightly. But this is platform-independent, so let's handle this
in a separate CL.

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1615: // undefined -> false
On 2011/08/05 12:50:46, Erik Corry wrote:
> Missing full stops on the end of comments several places.  Not your fault, but
> we might as well fix it now that we are looking at the code.

Done.

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1638: // Everything with a map could be undetectable,
so check this now.
On 2011/08/05 12:50:46, Erik Corry wrote:
> Pretty sure that it is just strings and JSObjects. ...

Again, this is platform-independent, so let's handle this in a separate CL.

http://codereview.chromium.org/7491054/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:1689: // internal objects -> true
On 2011/08/05 12:50:46, Erik Corry wrote:
> Caps and full stop.

Done.

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-arm.cc#newcode1044
src/arm/lithium-arm.cc:1044: // environment then.
On 2011/08/05 12:50:46, Erik Corry wrote:
> Do we hit this case in the tests?
> 
> I would guess it is so rare that the best way may be not to special case it.

Currently it happens quite often, because we fall back to "handle all" mode when
we haven't seen any type yet. This is done to avoid heavy performance
regressions caused by too many deoptimizations in these cases. So I propose
quite the opposite: Skip AssignEnvironment when the expected_input_types are
empty, too.

Let's reconsider this logic we handle these cases more intelligently.

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1589: // We've seen undefined for the first time
-> deopt.
On 2011/08/05 12:50:46, Erik Corry wrote:
> We should just bail out if we have seen an internal object.

See other comment regarding internal objects.

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1594: if
(expected.Contains(ToBooleanStub::BOOLEAN)) {
On 2011/08/05 12:50:46, Erik Corry wrote:
> This 'if' and the next can be merged.

This is a leftover from an attempt to do an "extract method" for oddballs, but
we have far too many free variables here to be useful.

I'll merge the ifs...

http://codereview.chromium.org/7491054/diff/1/src/arm/lithium-codegen-arm.cc#...
src/arm/lithium-codegen-arm.cc:1601: __ LoadRoot(ip, Heap::kTrueValueRootIndex);
On 2011/08/05 12:50:46, Erik Corry wrote:
> We have CompareRoot in the macro assembler.

... and I'm even using it below. :-P Using it in other places might in fact be a
cunning idea, too.