LGTM. I'm not sure this includes the addition of short external strings?
http://codereview.chromium.org/8569008/diff/5002/src/ia32/codegen-ia32.cc File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/8569008/diff/5002/src/ia32/codegen-ia32.cc#newcode566 src/ia32/codegen-ia32.cc:566: // Assert that the data pointer cache is valid. This shouldn't be necessary any more, right? The cache, if it is there, is always valid. Perhaps put this inside 'if (FLAG_debug_code) {' and make it a real Assert. http://codereview.chromium.org/8569008/diff/5002/src/x64/codegen-x64.cc File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/8569008/diff/5002/src/x64/codegen-x64.cc#newcode378 src/x64/codegen-x64.cc:378: __ movzxbl(result, FieldOperand(result, Map::kInstanceTypeOffset)); If we know that only strings get here, is it possible to expect the instance type in one of the input registers? We have probably loaded it at some point prior to getting here. http://codereview.chromium.org/8569008/diff/5002/src/x64/codegen-x64.cc#newcode382 src/x64/codegen-x64.cc:382: __ testb(result, Immediate(kIsIndirectStringMask)); Would it make sense to have kShortExternalStringMask as part of this mask? It is, after all, not a string that has direct access to character data. http://codereview.chromium.org/8569008/diff/5002/src/x64/codegen-x64.cc#newcode406 src/x64/codegen-x64.cc:406: __ movq(result, FieldOperand(string, ExternalString::kResourceDataOffset)); Should you be testing for short external strings here, before loading the resource? http://codereview.chromium.org/8569008/diff/5002/src/x64/codegen-x64.cc#newcode409 src/x64/codegen-x64.cc:409: __ j(zero, call_runtime); Not necessary? http://codereview.chromium.org/8569008/diff/5002/src/x64/codegen-x64.cc#newcode439 src/x64/codegen-x64.cc:439: // Check whether the string is sequential. The only non-sequential Comment seems a little off. When we get here, we know that the string is either a sequential or an external string. So perhaps write something like "Distinguish between sequential and external strings" to make it clear what's happening. http://codereview.chromium.org/8569008/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
