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<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<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<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<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<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<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<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<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/<http://codereview.chromium.org/8569008/> > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
