I only looked on architecture-independent parts and on ia32 code. Since
most of
the new generated code is not behind the flag we should make sure it's in a
good
shape. Let's discuss the slightly simplified charCodeAt code. After that
I'll
need one more pass.
-- Vitaly
http://codereview.chromium.org/7477045/diff/19020/src/heap.cc
File src/heap.cc (right):
http://codereview.chromium.org/7477045/diff/19020/src/heap.cc#newcode2698
src/heap.cc:2698: ASSERT(sliced_string->parent()->IsSeqString());
On 2011/08/11 13:23:43, Yang wrote:
On 2011/08/05 12:14:14, Vitaly Repeshko wrote:
> I think we can have flat cons with external first. It will pass the
> IsExternalString check above but will hit this assert.
Actually a flat cons with external first can't exist since flat cons
are only
created by SlowTryFlatten, which creates a new SeqString by
WriteToFlat. On the
other hand, if you externalize a flat cons, it is morphed into an
external
string.
Possible scenario:
1. a cons is flattened (its first component is a flat ASCII string now)
2. the first component is extracted (we do it in some places to improve
performance, could be hard to guard against)
3. the first component is externalized
http://codereview.chromium.org/7477045/diff/31001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/7477045/diff/31001/src/ia32/code-stubs-ia32.cc#newcode3393
src/ia32/code-stubs-ia32.cc:3393: #define NOT_SLICED -1
Ugh, use a constant instead.
http://codereview.chromium.org/7477045/diff/31001/src/ia32/code-stubs-ia32.cc#newcode3411
src/ia32/code-stubs-ia32.cc:3411: // Check for flat cons string or
truncated sliced string.
Update the comment.
http://codereview.chromium.org/7477045/diff/31001/src/ia32/code-stubs-ia32.cc#newcode3472
src/ia32/code-stubs-ia32.cc:3472: // edi: encoding of subject string (1
if ascii, 0 if two_byte);
Update the comment.
http://codereview.chromium.org/7477045/diff/31001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
http://codereview.chromium.org/7477045/diff/31001/src/ia32/lithium-codegen-ia32.cc#newcode3140
src/ia32/lithium-codegen-ia32.cc:3140: Register string =
ToRegister(instr->string());
Simplified version using three (writable) registers (untested):
Register string = ToRegister(instr->string());
Register index = ToRegister(instr->index);
Register result = ToRegister(instr->result());
DeferredStringCharCodeAt* deferred =
new DeferredStringCharCodeAt(this, instr);
// Fetch the instance type of the receiver into result register.
__ mov(result, FieldOperand(string, HeapObject::kMapOffset));
__ movzx_b(result, FieldOperand(result, Map::kInstanceTypeOffset));
// We need special handling for indirect strings.
Label check_sequential;
__ test(result, Immediate(kIsIndirectStringMask));
__ j(zero, &check_sequential, Label::kNear);
// Dispatch on the indirect string shape: slice or cons.
Label cons_string;
uint32_t sliced_not_cons_mask = kSlicedStringTag & ~kConsStringTag;
ASSERT(IsPowerOf2(sliced_not_cons_mask) && sliced_not_cons_mask != 0);
__ test(result, Immediate(sliced_not_cons_mask));
__ j(zero, &cons_string, Label::kNear);
// Handle slices.
Label indirect_string_loaded;
__ mov(result, FieldOperand(string, SlicedString::kOffsetOffset));
__ SmiUntag(result);
__ add(index, Operand(result));
__ mov(string, FieldOperand(string, SlicedString::kParentOffset));
__ jmp(&indirect_string_loaded, Label::kNear);
// Handle conses.
// Check whether the right hand side is the empty string (i.e. if
// this is really a flat string in a cons string). If that is not
// the case we would rather go to the runtime system now to flatten
// the string.
__ bind(&cons_string);
__ cmp(FieldOperand(string, ConsString::kSecondOffset),
Immediate(factory()->empty_string()));
__ j(not_equal, deferred->entry());
__ mov(string, FieldOperand(string, ConsString::kFirstOffset));
__ bind(&indirect_string_loaded);
__ mov(result, FieldOperand(string, HeapObject::kMapOffset));
__ movzx_b(result, FieldOperand(result, Map::kInstanceTypeOffset));
// Check whether the string is sequential. The only non-sequential
// shapes we support have just been unwrapped above.
__ bind(&check_sequential);
STATIC_ASSERT(kSeqStringTag == 0);
__ test(result, Immediate(kStringRepresentationMask));
__ j(not_zero, deferred->entry());
// Dispatch on the encoding: ASCII or two-byte.
Label ascii_string;
STATIC_ASSERT(kAsciiStringTag != 0);
__ test(result, Immediate(kStringEncodingMask));
__ j(not_zero, &ascii_string, Label::kNear);
// Two-byte string.
// Load the two-byte character code into the result register.
Label done;
STATIC_ASSERT(kSmiTag == 0 && kSmiTagSize == 1);
__ movzx_w(result, FieldOperand(string,
index,
times_2,
SeqTwoByteString::kHeaderSize));
__ jmp(&done, Label::kNear);
// ASCII string.
// Load the byte into the result register.
__ bind(&ascii_string);
__ movzx_b(result, FieldOperand(string,
index,
times_1,
SeqAsciiString::kHeaderSize));
__ bind(&done);
__ bind(deferred->exit());
http://codereview.chromium.org/7477045/diff/31001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
http://codereview.chromium.org/7477045/diff/31001/src/ia32/lithium-ia32.cc#newcode2060
src/ia32/lithium-ia32.cc:2060: LOperand* string =
UseTempRegister(instr->string());
We can use 3 instead of 4 registers if we give up on special casing the
const index:
LOperand* string = UseTempRegister(instr->string());
LOperand* index = UseTempRegister(instr->string());
LOperand* context = UseAny(instr->context());
LStringCharCodeAt* result = new LStringCharCodeAt(context, string,
index);
http://codereview.chromium.org/7477045/diff/31001/src/ia32/regexp-macro-assembler-ia32.cc
File src/ia32/regexp-macro-assembler-ia32.cc (right):
http://codereview.chromium.org/7477045/diff/31001/src/ia32/regexp-macro-assembler-ia32.cc#newcode1073
src/ia32/regexp-macro-assembler-ia32.cc:1073: String* subject_ptr =
*subject;
Should be put in a handle.
http://codereview.chromium.org/7477045/diff/31001/src/regexp-macro-assembler.cc
File src/regexp-macro-assembler.cc (right):
http://codereview.chromium.org/7477045/diff/31001/src/regexp-macro-assembler.cc#newcode135
src/regexp-macro-assembler.cc:135:
ASSERT(slice->parent()->IsSeqString());
Should this be here? Looks like we can handle external strings fine
below.
http://codereview.chromium.org/7477045/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
