Port number dictionary probing in generated code to ARM....

Port number dictionary probing in generated code to ARM.

Fix bug in ARM pixel array load code and a typo in the x64 number
dictionary load code.

Fix bug in string dictionary probing where we did not bail out if the
object has an interceptor.

BUG=640
Committed: http://code.google.com/p/v8/source/detail?r=4292

[Vyacheslav Egorov (Chromium), 2010-03-25 19:59:34]

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.

[Vyacheslav Egorov (Chromium), 2010-03-25 20:07:11]

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));
ComputeIntegerHash uses uint32_t hash, so shifts are logical not arithmetic. 

It was correct in the first CL.

[Mads Ager (chromium), 2010-03-26 07:40:20]

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. :)

[Vyacheslav Egorov (Chromium), 2010-03-26 09:00:28]

LGTM

http://codereview.chromium.org/1332003/diff/11001/12003
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/1332003/diff/11001/12003#newcode77
src/x64/ic-x64.cc:77: __ testl(FieldOperand(r0, Map::kBitFieldOffset),
Unaligned read. Is it good? 
 
Version with tricky bit manipulations would be X cycles faster [and Y times more
fragile] ;-)