Yeah, I noticed when I got to the other one :) At least I could reuse some of my comments! /L
On Thu, Nov 24, 2011 at 11:17, Yang Guo <[email protected]> wrote: > I'm sorry. I forgot to point out that this has been replaced by my other CL > that ports both r10023 and r10054. > > On Thu, Nov 24, 2011 at 10:21 AM, <[email protected]> wrote: >> >> 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/ > > -- Lasse R.H. Nielsen [email protected] 'Faith without judgement merely degrades the spirit divine' Google Denmark ApS - Frederiksborggade 20B, 1 sal - 1360 København K - Denmark - CVR nr. 28 86 69 84 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
