Mads, I have some style questions/ramblings and one serious question [see ic-x64.cc for serious question].
http://codereview.chromium.org/1332003/diff/1/3 File src/arm/ic-arm.cc (right): http://codereview.chromium.org/1332003/diff/1/3#newcode148 src/arm/ic-arm.cc:148: Label* miss, It seemed to me that miss/fail label usually goes last. But I might be wrong. http://codereview.chromium.org/1332003/diff/1/3#newcode176 src/arm/ic-arm.cc:176: __ mvn(t0, Operand(t0)); Can we do __ mvn(t1, Operand(t0)) __ add(t0, t1, Operand(t0, LSL, 15)) ? It seems to save one instruction (no mov (t1, t0)). http://codereview.chromium.org/1332003/diff/1/3#newcode191 src/arm/ic-arm.cc:191: const int kCapacityOffset = This constant seems to be meaningful for all architectures. Maybe put it into NumberDictionary? http://codereview.chromium.org/1332003/diff/1/3#newcode198 src/arm/ic-arm.cc:198: const int kElementsStartOffset = Ditto http://codereview.chromium.org/1332003/diff/1/3#newcode207 src/arm/ic-arm.cc:207: // Compute the masked index: (hash + i + i * i) & mask. Comment contains (inlined) information about GetProbeOffset(i) Is it relevant here? If GetProbeOffset changes somebody will have to reinline the comment... Which never happens. On the second thought: yeah, it is relevant because we can see that GetProbeOffset(0) = 0 and we can skip addition... Still seems fragile to me. http://codereview.chromium.org/1332003/diff/1/3#newcode231 src/arm/ic-arm.cc:231: const int kDetailsOffset = kElementsStartOffset + 2 * kPointerSize; Ditto [constant is meaningful for all platforms] http://codereview.chromium.org/1332003/diff/1/3#newcode233 src/arm/ic-arm.cc:233: __ tst(ip, Operand(Smi::FromInt(PropertyDetails::TypeField::mask()))); Is it necessary to use ip here? t1 seems to be free at this point. ip might bite us if Operand(...) stop fitting in fancy imm8-rotated encoding. [nothing fatal though, there is a CHECK to catch it in addrmode1] http://codereview.chromium.org/1332003/diff/1/3#newcode697 src/arm/ic-arm.cc:697: __ b(ne, &slow); It's unclear for me why something having HashTable map must be NumberDictionary. It seems to me that nothing prevents us from having say StringDictionary in this field. http://codereview.chromium.org/1332003/diff/1/2 File src/x64/ic-x64.cc (left): http://codereview.chromium.org/1332003/diff/1/2#oldcode206 src/x64/ic-x64.cc:206: StringDictionary::kCapacityIndex * kPointerSize; I sense mighty copy-paste here. ;-) Which implies that this code could be rewritten in a generic fashion [function generating code for abstract dictionary lookup probing]. http://codereview.chromium.org/1332003/diff/1/2 File src/x64/ic-x64.cc (right): http://codereview.chromium.org/1332003/diff/1/2#newcode77 src/x64/ic-x64.cc:77: Immediate(1 << (Map::kHasNamedInterceptor + (3 * 8)))); Hmmm, I am not getting this code. flag kHasNamedInterceptor is in bit_field(), which has offset kBitFieldOffset = kInstanceAttributesOffset + 2. 8 == kBitsPerByte, but 3 != kBitFieldOffset - kInstanceAttributeOffset = 2 In any case magic constants are evil. http://codereview.chromium.org/1332003 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev To unsubscribe from this group, send email to v8-dev+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
