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#newcode1613 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#newcode1615 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#newcode1638 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#newcode1689 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#newcode1589 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#newcode1594 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#newcode1601 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. http://codereview.chromium.org/7491054/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
