Remove redundant checks in and around GenerateDictionaryLoad....

Remove redundant checks in and around GenerateDictionaryLoad.

Similar or duplicate checks are scattered around the code before doing the dictionary load. 
Also the entire branch in GenerateCallNormal that handles global/builtin receiver is 
guaranteed to bail out from GenerateDictionaryLoad, so there is no point in generating it at all.

The purpose of the patch is:
- making C++ code more compact and transparent,
- not generating dead code. 

There is a tiny performance gain. The patch is ia32 only for now.

Please tell me if I am missing anything.



Committed: http://code.google.com/p/v8/source/detail?r=4926

[Mads Ager (chromium), 2010-06-17 11:37:04]

I like the cleanup in this changelist.

http://codereview.chromium.org/2801007/diff/1/2
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2801007/diff/1/2#newcode98
src/ia32/ic-ia32.cc:98: // Appropriate access checks must be done before
entering this code.
You need to spell out what appropriate checks mean here. We need to check that
the receiver is not a global object and that it does not have interceptors.

http://codereview.chromium.org/2801007/diff/1/2#newcode1213
src/ia32/ic-ia32.cc:1213: static void GenerateFunctionCall(MacroAssembler* masm,
int argc, Label* miss) {
GenerateFunctionCall makes it sound like the code will return to after
GenerateFunctionCall. Rename to GenerateFunctionTailCall or GenerateJumpFunction
or something. In the macro assembler we use CallX and TailCallX:
CallStub/TailCallStub, CallRuntime/TailCallRuntime.

http://codereview.chromium.org/2801007/diff/1/2#newcode1250
src/ia32/ic-ia32.cc:1250: GenerateLoadReceiverCheck(masm, edx, ebx, eax, &miss,
&global_proxy);
Thanks for cleaning this up! 

I think we should clean it up even more and bail out on global proxy objects as
well? The proxy object does not contains any properties on its own and the
dictionary probing code used to bail out on global proxy objects. The current
change seems to allow dictionary probing of proxy objects which does not make
sense (it probably bails out because the properties are not a dictionary, but we
should avoid the attempt completely).

http://codereview.chromium.org/2801007/diff/1/2#newcode1304
src/ia32/ic-ia32.cc:1304: // This can happen only for regular CallIC but not
KeyedCallIC.
Why can't this happen for KeyedCallICs? Because we always do a
LoadGlobalReceiver before calling KeyedCallICs (and a LoadGlobal before calling
CallICs)? It would be very bad to miss the receiver patching for keyed call ics,
so we need to be 100% sure that this is correct. The best way to convince
ourself that this is correct is to make sure that there is a test for both
CallICs and KeyedCallICs where we hit this path and the patching matters (access
checks should fail).

http://codereview.chromium.org/2801007/diff/1/2#newcode1539
src/ia32/ic-ia32.cc:1539: GenerateLoadReceiverCheck(masm, eax, ebx, edx, &miss,
&global);
Same comment: Thanks for cleaning this up. We should miss on global proxy as
well.

[Mads Ager (chromium), 2010-06-23 07:18:12]

LGTM!

http://codereview.chromium.org/2801007/diff/5001/6002
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/2801007/diff/5001/6002#newcode53
src/arm/ic-arm.cc:53: Label* miss) {
miss -> global_object?

http://codereview.chromium.org/2801007/diff/5001/6002#newcode65
src/arm/ic-arm.cc:65: // Generated code falls through if the receiver is a
regular JS object
regular -> regular non-global

http://codereview.chromium.org/2801007/diff/5001/6002#newcode98
src/arm/ic-arm.cc:98: __ b(ne, miss);
You could you nz here (which is the same as ne so it only matters for
readability).

http://codereview.chromium.org/2801007/diff/5001/6002#newcode126
src/arm/ic-arm.cc:126: Register t0,
Change name back to scratch for these?

http://codereview.chromium.org/2801007/diff/5001/6002#newcode130
src/arm/ic-arm.cc:130: //           dictionary.
Indentation or change name of registers back to scratch.

http://codereview.chromium.org/2801007/diff/5001/6002#newcode347
src/arm/ic-arm.cc:347: __ b(ne, slow);
Could use nz.

[Vladislav Kaznacheev, 2010-06-23 09:06:18]

http://codereview.chromium.org/2801007/diff/1/2
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/2801007/diff/1/2#newcode98
src/ia32/ic-ia32.cc:98: // Appropriate access checks must be done before
entering this code.
On 2010/06/17 11:37:04, Mads Ager wrote:
> You need to spell out what appropriate checks mean here. We need to check that
> the receiver is not a global object and that it does not have interceptors.

Done.

http://codereview.chromium.org/2801007/diff/1/2#newcode1213
src/ia32/ic-ia32.cc:1213: static void GenerateFunctionCall(MacroAssembler* masm,
int argc, Label* miss) {
On 2010/06/17 11:37:04, Mads Ager wrote:
> GenerateFunctionCall makes it sound like the code will return to after
> GenerateFunctionCall. Rename to GenerateFunctionTailCall or
GenerateJumpFunction
> or something. In the macro assembler we use CallX and TailCallX:
> CallStub/TailCallStub, CallRuntime/TailCallRuntime.

Done.

http://codereview.chromium.org/2801007/diff/1/2#newcode1250
src/ia32/ic-ia32.cc:1250: GenerateLoadReceiverCheck(masm, edx, ebx, eax, &miss,
&global_proxy);
On 2010/06/17 11:37:04, Mads Ager wrote:
> Thanks for cleaning this up! 
> 
> I think we should clean it up even more and bail out on global proxy objects
as
> well? The proxy object does not contains any properties on its own and the
> dictionary probing code used to bail out on global proxy objects. The current
> change seems to allow dictionary probing of proxy objects which does not make
> sense (it probably bails out because the properties are not a dictionary, but
we
> should avoid the attempt completely).

Done.

http://codereview.chromium.org/2801007/diff/1/2#newcode1304
src/ia32/ic-ia32.cc:1304: // This can happen only for regular CallIC but not
KeyedCallIC.
Before introducing KeyedCallIC keyed call were done via CallFunctionStub that
did not patch global receiver and everything worked fine. KeyedCallIC just
replaced CallFunctionStub so global object should not show up here.

On 2010/06/17 11:37:04, Mads Ager wrote:
> Why can't this happen for KeyedCallICs? Because we always do a
> LoadGlobalReceiver before calling KeyedCallICs (and a LoadGlobal before
calling
> CallICs)? It would be very bad to miss the receiver patching for keyed call
ics,
> so we need to be 100% sure that this is correct. The best way to convince
> ourself that this is correct is to make sure that there is a test for both
> CallICs and KeyedCallICs where we hit this path and the patching matters
(access
> checks should fail).

http://codereview.chromium.org/2801007/diff/1/2#newcode1539
src/ia32/ic-ia32.cc:1539: GenerateLoadReceiverCheck(masm, eax, ebx, edx, &miss,
&global);
On 2010/06/17 11:37:04, Mads Ager wrote:
> Same comment: Thanks for cleaning this up. We should miss on global proxy as
> well.

Done.

http://codereview.chromium.org/2801007/diff/5001/6002
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/2801007/diff/5001/6002#newcode53
src/arm/ic-arm.cc:53: Label* miss) {
Done (for all 3 platforms)

On 2010/06/23 07:18:12, Mads Ager wrote:
> miss -> global_object?

http://codereview.chromium.org/2801007/diff/5001/6002#newcode65
src/arm/ic-arm.cc:65: // Generated code falls through if the receiver is a
regular JS object
On 2010/06/23 07:18:12, Mads Ager wrote:
> regular -> regular non-global
Done (for all 3 platforms)

http://codereview.chromium.org/2801007/diff/5001/6002#newcode98
src/arm/ic-arm.cc:98: __ b(ne, miss);
On 2010/06/23 07:18:12, Mads Ager wrote:
> You could you nz here (which is the same as ne so it only matters for
> readability).

Done.

http://codereview.chromium.org/2801007/diff/5001/6002#newcode126
src/arm/ic-arm.cc:126: Register t0,
On 2010/06/23 07:18:12, Mads Ager wrote:
> Change name back to scratch for these?

Done.

http://codereview.chromium.org/2801007/diff/5001/6002#newcode130
src/arm/ic-arm.cc:130: //           dictionary.
On 2010/06/23 07:18:12, Mads Ager wrote:
> Indentation or change name of registers back to scratch.

Done.

http://codereview.chromium.org/2801007/diff/5001/6002#newcode347
src/arm/ic-arm.cc:347: __ b(ne, slow);
On 2010/06/23 07:18:12, Mads Ager wrote:
> Could use nz.

Done.