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, On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
It seemed to me that miss/fail label usually goes last. But I might be
wrong. This is consistent with the other platforms and we are not consistently putting the miss label at the end (but probably most of the time). http://codereview.chromium.org/1332003/diff/1/3#newcode176 src/arm/ic-arm.cc:176: __ mvn(t0, Operand(t0)); On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
Can we do
__ mvn(t1, Operand(t0)) __ add(t0, t1, Operand(t0, LSL, 15))
?
It seems to save one instruction (no mov (t1, t0)).
Yes, we can! Thanks. http://codereview.chromium.org/1332003/diff/1/3#newcode191 src/arm/ic-arm.cc:191: const int kCapacityOffset = On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
This constant seems to be meaningful for all architectures.
Maybe put it into NumberDictionary?
It is actually already there. Done. http://codereview.chromium.org/1332003/diff/1/3#newcode198 src/arm/ic-arm.cc:198: const int kElementsStartOffset = On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
Ditto
Done. http://codereview.chromium.org/1332003/diff/1/3#newcode207 src/arm/ic-arm.cc:207: // Compute the masked index: (hash + i + i * i) & mask. On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
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. It is as you say fragile. But at least it is explicit and if someone changes GetProbeOffset the comment might help realize the assumptions that we are making when adapting this code. http://codereview.chromium.org/1332003/diff/1/3#newcode233 src/arm/ic-arm.cc:233: __ tst(ip, Operand(Smi::FromInt(PropertyDetails::TypeField::mask()))); On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
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]
Done. It is better to not use ip here. http://codereview.chromium.org/1332003/diff/1/3#newcode697 src/arm/ic-arm.cc:697: __ b(ne, &slow); On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
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.
Nothing technically prevents us from having a StringDictionary in this field. However, this field is the elements field of a JSObject with can only contain either a FixedArray or a NumberDictionary (which just has a HashTable map). 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; On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
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].
Yeah, there are some minor differences in the loop that made me think that the generic function would get convoluted. That said, I will play with refactoring this - at least the code checking if the result loaded from the string/number dictionary is a normal property should to shared. I will do that as a separate change. 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)))); On 2010/03/25 19:59:34, Vyacheslav Egorov wrote:
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.
This is clearly wrong. Very well spotted! I was not looking at the code - just trying to fix indentation. :) I have fixed this on all platforms and added a regression test. http://codereview.chromium.org/1332003/diff/5001/6002 File src/arm/ic-arm.cc (right): http://codereview.chromium.org/1332003/diff/5001/6002#newcode179 src/arm/ic-arm.cc:179: __ eor(t0, t0, Operand(t0, ASR, 12)); On 2010/03/25 20:07:11, Vyacheslav Egorov wrote:
ComputeIntegerHash uses uint32_t hash, so shifts are logical not
arithmetic.
It was correct in the first CL.
Yes, thanks Slava! This was actually the only thing that I changed for the second patch set and you are right, I got it right the first time. :) 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.
