Break Crankshaft into phases.

Break Crankshaft into phases.

Crankshaft now runs by calling CreateGraph on the HGraphBuilder, then
calling Optimize and Codegen on the HGraph.

BUG=
TEST=

Committed: https://code.google.com/p/v8/source/detail?r=12064

[sanjoy, 2012-07-05 13:23:57]

http://codereview.chromium.org/10700115/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10700115/diff/1/src/hydrogen.cc#newcode3097
src/hydrogen.cc:3097: LChunk* HGraph::Optimize() {
Once the representation change CL is in, it is safe to run Optimize in a
NoHandleAllocation scope.

[danno, 2012-07-11 08:20:23]

http://codereview.chromium.org/10700115/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/10700115/diff/1/src/hydrogen.h#newcode248
src/hydrogen.h:248: HGraph(CompilationInfo* info, HGraphBuilder* graph_builder);
Don't pass graph_builder here, see longer comment below.

http://codereview.chromium.org/10700115/diff/1/src/hydrogen.h#newcode317
src/hydrogen.h:317: LChunk* Optimize();
You have introduced a "layer violation" with this change. HGraph should not have
a dependency on anything except for hydrogen instructions. However, with this
change you introduce a dependency on the graph builder, which isn't a good idea
because the graph shouldn't have knowledge of how it's built, just what it has
inside of itself (for example, I am implementing stub generation that uses
HGraph, but it doesn't use the HGraphBuilder, which is only used to generate
optimized JavaScript functions).  You also strengthen an existing dependency on
chunks/chunk builder, depending on the Lithium layer from Hydrogen. I'd like to
untangle that dependency a little bit if you are moving code already.

Concrete suggestion:
- Don't keep a pointer to the graph builder in the graph.
- Separate the async work into two methods. One on the graph called optimize,
which somehow communicates to the caller if it fails due to a bailout, and the
caller handles the bailout because it has a reference to the graph builder.
Another one called PrepareCodegen, perhaps on the Compiler object, that does the
work that you currently have at the end of optimize. 
- Make the asynchronous part of compilation call "graph()->Optimized() and
PrepareCodegen()"

[sanjoy, 2012-07-11 09:55:53]

Moved the lithium-specific code into lithium.cc and lithium.h.

http://codereview.chromium.org/10700115/diff/7001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10700115/diff/7001/src/hydrogen.cc#newcode40
src/hydrogen.cc:40: #if V8_TARGET_ARCH_IA32
This #include block is needed only by HTracer now.

[danno, 2012-07-11 10:04:57]

http://codereview.chromium.org/10700115/diff/7001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10700115/diff/7001/src/compiler.cc#newcode315
src/compiler.cc:315: LChunk* chunk = Lithium::CreateChunk(graph, &builder);
How about LChunk::NewChunk?

http://codereview.chromium.org/10700115/diff/7001/src/compiler.cc#newcode317
src/compiler.cc:317: Handle<Code> optimized_code = Lithium::Codegen(chunk);
How about chunk->Codegen(); ?

http://codereview.chromium.org/10700115/diff/7001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10700115/diff/7001/src/hydrogen.cc#newcode3075
src/hydrogen.cc:3075: bool HGraph::Optimize(const char** bailout_reason) {
Memory management/owner is totally unclear for the bailout reason. Shouldn't you
pass this via a shared string or equivalent?

http://codereview.chromium.org/10700115/diff/7001/src/lithium.cc
File src/lithium.cc (right):

http://codereview.chromium.org/10700115/diff/7001/src/lithium.cc#newcode257
src/lithium.cc:257: LChunk* Lithium::CreateChunk(HGraph* graph, HGraphBuilder*
graph_builder) {
Again, adding a dependency on graph builder here is only due to the bailout, and
that doesn't seem right. Pass failure reason back out.

http://codereview.chromium.org/10700115/diff/7001/src/lithium.h
File src/lithium.h (right):

http://codereview.chromium.org/10700115/diff/7001/src/lithium.h#newcode627
src/lithium.h:627: static LChunk* CreateChunk(HGraph* graph, HGraphBuilder*
builder);
Why not make this a static method on LChunk?

http://codereview.chromium.org/10700115/diff/7001/src/lithium.h#newcode628
src/lithium.h:628: static Handle<Code> Codegen(LChunk* chunk);
Can you make this a method on the chunk?

[sanjoy, 2012-07-11 11:12:34]

I considered adding the methods to LChunk, but then noticed that it is an
architecture-specific class.  I'd end up duplicating the same code (which isn't
inherently arch-specific) in a bunch of different files.

[danno, 2012-07-12 08:38:02]

http://codereview.chromium.org/10700115/diff/3009/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10700115/diff/3009/src/hydrogen.cc#newcode3087
src/hydrogen.cc:3087: bool HGraph::Optimize(SmartArrayPointer<char>*
deopt_reason) {
Should be "bailout_reason"

http://codereview.chromium.org/10700115/diff/3009/src/lithium.h
File src/lithium.h (right):

http://codereview.chromium.org/10700115/diff/3009/src/lithium.h#newcode676
src/lithium.h:676: static Handle<Code> Codegen(LChunk* chunk);
Why do you need to make this static? If you make it an instance method, you
don't need the wrapper methods that call through in the subclasses.

[sanjoy, 2012-07-12 08:50:13]

On 2012/07/12 08:38:02, danno wrote:
> http://codereview.chromium.org/10700115/diff/3009/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
> http://codereview.chromium.org/10700115/diff/3009/src/hydrogen.cc#newcode3087
> src/hydrogen.cc:3087: bool HGraph::Optimize(SmartArrayPointer<char>*
> deopt_reason) {
> Should be "bailout_reason"
> 
> http://codereview.chromium.org/10700115/diff/3009/src/lithium.h
> File src/lithium.h (right):
> 
> http://codereview.chromium.org/10700115/diff/3009/src/lithium.h#newcode676
> src/lithium.h:676: static Handle<Code> Codegen(LChunk* chunk);
> Why do you need to make this static? If you make it an instance method, you
> don't need the wrapper methods that call through in the subclasses.

This is because LCodeGen works on an LChunk and not an LChunkBase (and
this dependency is not syntactic -- it access fields added by LChunk).

[sanjoy, 2012-07-12 09:06:49]

http://codereview.chromium.org/10700115/diff/3009/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10700115/diff/3009/src/hydrogen.cc#newcode3087
src/hydrogen.cc:3087: bool HGraph::Optimize(SmartArrayPointer<char>*
deopt_reason) {
On 2012/07/12 08:38:03, danno wrote:
> Should be "bailout_reason"

Done.

http://codereview.chromium.org/10700115/diff/3009/src/lithium.h
File src/lithium.h (right):

http://codereview.chromium.org/10700115/diff/3009/src/lithium.h#newcode676
src/lithium.h:676: static Handle<Code> Codegen(LChunk* chunk);
On 2012/07/12 08:38:03, danno wrote:
> Why do you need to make this static? If you make it an instance method, you
> don't need the wrapper methods that call through in the subclasses.

As I mentioned, LCodeGen specifically needs an LChunk.

[danno, 2012-07-12 11:14:54]

https://chromiumcodereview.appspot.com/10700115/diff/6/src/compiler.cc
File src/compiler.cc (right):

https://chromiumcodereview.appspot.com/10700115/diff/6/src/compiler.cc#newcode51
src/compiler.cc:51: #include "ia32/lithium-ia32.h"
Why do you need these? At this point, all of the calls that you make to LChunk
and co. shouldn't require the platform specific headers, right?

https://chromiumcodereview.appspot.com/10700115/diff/6/src/mips/lithium-mips.h
File src/mips/lithium-mips.h (right):

https://chromiumcodereview.appspot.com/10700115/diff/6/src/mips/lithium-mips....
src/mips/lithium-mips.h:2200: return reinterpret_cast<LChunk*>(chunk_base);
I am not sure if this (and others in ia32/x64/ARM) add any value. Elsewhere in
the runtime, the cast function does additional runtime checks to make sure the
cast is valid, justifying the existence of the extra function. Here, no such
check is possible, so I think it's probably better/simpler/less code just to do
the static_cast directly in the LCodeGen constructor.

[sanjoy, 2012-07-12 11:34:18]

https://chromiumcodereview.appspot.com/10700115/diff/6/src/compiler.cc
File src/compiler.cc (right):

https://chromiumcodereview.appspot.com/10700115/diff/6/src/compiler.cc#newcode51
src/compiler.cc:51: #include "ia32/lithium-ia32.h"
On 2012/07/12 11:14:54, danno wrote:
> Why do you need these? At this point, all of the calls that you make to LChunk
> and co. shouldn't require the platform specific headers, right?

The register allocator needed an LChunk to operate on.  Fixed this by getting to
work with an LChunkBase instead.

https://chromiumcodereview.appspot.com/10700115/diff/6/src/mips/lithium-mips.h
File src/mips/lithium-mips.h (right):

https://chromiumcodereview.appspot.com/10700115/diff/6/src/mips/lithium-mips....
src/mips/lithium-mips.h:2200: return reinterpret_cast<LChunk*>(chunk_base);
On 2012/07/12 11:14:54, danno wrote:
> I am not sure if this (and others in ia32/x64/ARM) add any value. Elsewhere in
> the runtime, the cast function does additional runtime checks to make sure the
> cast is valid, justifying the existence of the extra function. Here, no such
> check is possible, so I think it's probably better/simpler/less code just to
do
> the static_cast directly in the LCodeGen constructor.

Done.

[danno, 2012-07-12 15:00:08]

LGTM with comment/nit, please address before landing.

https://chromiumcodereview.appspot.com/10700115/diff/11005/src/ia32/lithium-i...
File src/ia32/lithium-ia32.h (left):

https://chromiumcodereview.appspot.com/10700115/diff/11005/src/ia32/lithium-i...
src/ia32/lithium-ia32.h:2330: 
Remove this whitespace change, please.