Add complete implementation of full compiler for the ia32 architecture...

Add complete implementation of full compiler for the ia32 architecture

This makes the full compiler handle all constructs on ia32. However the syntax checker for the full compiler is still the same so for both normal operation and with the flag --always-full-compiler the coverage of the full compiler will be the same.

This is on preparation for improving the debugger break point experience where the plan is to only use code from full code generator when debugging JavaScript.

Runs all tests on all three platforms in release and debug mode. The tests also run with both the following flags to the test runner

  --special-command="@ --nofull-compiler"
  --special-command="@ --always-full-compiler"

The changes to the x64 and ARM architectures are mainly structural due to the change to EmitVariableAssignment to handle initialization of const variables.

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

[Søren Thygesen Gjesse, 2010-05-12 08:25:32]

I suggest the following division of labour:

fschneider: Please look at the ia32 part and the platform independent parts

lrn: Please look at the x64 part

erik.corry: Please look at the ARM part

antonm: Please look at FullCodeGenerator::EmitGetFromCache. In there I have
adapted the code from the defered code object DeferredSearchCache in the virtual
frame based compiler between #if 0 and #endif, and I currently call
Runtime_GetFromCache instead of using that code. The code between #if 0 and
#endif pass all the tests, so do you think it makes sense to have that code in
the full compiler as well as in the virtual frame based one or should we rely on
the single use of %_GetFromCache in strings.js being compiled by the virtual
frame based compiler? Maybe just moving the code from defered code object
DeferredSearchCache into a stub will work and we can use it from both places.

[Lasse Reichstein, 2010-05-12 10:57:23]

X64 LGTM (but it's hard to see the context from looking just at that).

[antonm, 2010-05-12 11:02:19]

my part LGTM

http://codereview.chromium.org/1989012/diff/9001/10006
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/1989012/diff/9001/10006#newcode2568
src/ia32/full-codegen-ia32.cc:2568: __ CallRuntime(Runtime::kSwapElements, 3);
runtime call here might be quite expensive.  I don't have any numbers, but when
x64 port had only runtime call w/o inlined version, there was a notable
regression.

http://codereview.chromium.org/1989012/diff/9001/10006#newcode2620
src/ia32/full-codegen-ia32.cc:2620: // it make sense to use that here instead of
the runtime call?
runtime call might be fine most of the time.  The only problem: if we have cache
miss invocation of JS function from C++ might be unnecessary expensive.  I think
that the stub is the best solution.  I didn't implement it in the first place to
keep pieces digestible.

http://codereview.chromium.org/1989012/diff/9001/10006#newcode2624
src/ia32/full-codegen-ia32.cc:2624: STATIC_ASSERT(kSmiTag == 0);
if you're going to uncomment this code, those assertions are probably
unnecessary

[fschneider, 2010-05-12 12:03:51]

LGTM from my side.

http://codereview.chromium.org/1989012/diff/9001/10001
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/1989012/diff/9001/10001#newcode1105
src/arm/full-codegen-arm.cc:1105: __ push(r0);
Could you use the stm instruction here for more compact code?

http://codereview.chromium.org/1989012/diff/9001/10003
File src/compiler.cc (right):

http://codereview.chromium.org/1989012/diff/9001/10003#newcode136
src/compiler.cc:136: #ifdef V8_TARGET_ARCH_IA32
Maybe there is no need for this #ifdef? The FastCodeGen is hidden behind a flag 
and not enabled by default. We could also consider removing it completely for
now (as a separate change).

http://codereview.chromium.org/1989012/diff/9001/10004
File src/full-codegen.cc (right):

http://codereview.chromium.org/1989012/diff/9001/10004#newcode58
src/full-codegen.cc:58: #ifndef V8_TARGET_ARCH_IA32
Maybe move this #ifndef to the call site of the syntax checker (until the other
platforms are completed)?

[Erik Corry, 2010-05-12 12:04:26]

ARM port LGTM

[Søren Thygesen Gjesse, 2010-05-19 07:24:53]

Reverted the changes in compiler.cc, so that the coverage of the full compiler
on all three platforms is the same and unchanged by this change.

I will make a follow up change where the full compiler on ia32 will take full
effect when debugging is enabled.

http://codereview.chromium.org/1989012/diff/9001/10001
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/1989012/diff/9001/10001#newcode1105
src/arm/full-codegen-arm.cc:1105: __ push(r0);
On 2010/05/12 12:03:52, fschneider wrote:
> Could you use the stm instruction here for more compact code?

Done (used Push(cp, r0)).

http://codereview.chromium.org/1989012/diff/9001/10003
File src/compiler.cc (right):

http://codereview.chromium.org/1989012/diff/9001/10003#newcode136
src/compiler.cc:136: #ifdef V8_TARGET_ARCH_IA32
On 2010/05/12 12:03:52, fschneider wrote:
> Maybe there is no need for this #ifdef? The FastCodeGen is hidden behind a
flag 
> and not enabled by default. We could also consider removing it completely for
> now (as a separate change).

Removed, that was a mistake. I did not intend to mess with the fast compiler.

http://codereview.chromium.org/1989012/diff/9001/10004
File src/full-codegen.cc (right):

http://codereview.chromium.org/1989012/diff/9001/10004#newcode58
src/full-codegen.cc:58: #ifndef V8_TARGET_ARCH_IA32
On 2010/05/12 12:03:52, fschneider wrote:
> Maybe move this #ifndef to the call site of the syntax checker (until the
other
> platforms are completed)?
> 

Done.

http://codereview.chromium.org/1989012/diff/9001/10006
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/1989012/diff/9001/10006#newcode2568
src/ia32/full-codegen-ia32.cc:2568: __ CallRuntime(Runtime::kSwapElements, 3);
On 2010/05/12 11:02:20, antonm wrote:
> runtime call here might be quite expensive.  I don't have any numbers, but
when
> x64 port had only runtime call w/o inlined version, there was a notable
> regression.

We have revised the current scope of the full scope of the full compiler, so
that it will only be in full effect when debugging otherwise it will keep the
scope currently defined by the syntex checker. Therefore having a runtime call
here should be ok.

http://codereview.chromium.org/1989012/diff/9001/10006#newcode2620
src/ia32/full-codegen-ia32.cc:2620: // it make sense to use that here instead of
the runtime call?
On 2010/05/12 11:02:20, antonm wrote:
> runtime call might be fine most of the time.  The only problem: if we have
cache
> miss invocation of JS function from C++ might be unnecessary expensive.  I
think
> that the stub is the best solution.  I didn't implement it in the first place
to
> keep pieces digestible.

See comment above. Keeping runtime call.

http://codereview.chromium.org/1989012/diff/9001/10006#newcode2624
src/ia32/full-codegen-ia32.cc:2624: STATIC_ASSERT(kSmiTag == 0);
On 2010/05/12 11:02:20, antonm wrote:
> if you're going to uncomment this code, those assertions are probably
> unnecessary

This code will be removed.