Introduce an OptimizingCompiler class, responsible for maintaining the state needed to run Cranksha…

Introduce an OptimizingCompiler class, responsible for maintaining the state needed to run Crankshaft.


BUG=
TEST=

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

[danno, 2012-07-16 12:19:09]

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode230
src/compiler.cc:230: Timer t(this);
nit: Move this down after the comment so diff is clearer.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode268
src/compiler.cc:268: Handle<Context> global_context(
This fits on one line like in the original, no?

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode281
src/compiler.cc:281: 
Don't add a line here, please keep original white space as much as possible to
help make diff clearer.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode297
src/compiler.cc:297: return BAILED_OUT;
How about AbortCrankshaft just returns a OptimizingCompiler::Status so you can
write return AbortCrankshaft();

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode301
src/compiler.cc:301: if (chunk_ == NULL) {
Why will chunk ever be null? It seems like the only path is in the else above,
why not put the test there and always return SUCCEEDED at the end of this
funciton?

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode324
src/compiler.cc:324: void OptimizingCompiler::RecordOptimizationStats() {
Can you please move this back up to where FinishOptimization used to be? Makes
the diffs much simpler.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode352
src/compiler.cc:352: static bool MakeCrankshaftCode(CompilationInfo* info) {
Can you please move this up where MakeCrankshaftCode used to be? Again, to
minimize the diffs if possible.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode319
src/compiler.h:319: // A helper class that calls the three phases in Crankshaft
and keeps
nit: compilation phases
maybe you should list what they are?

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode337
src/compiler.h:337: Status CreateHGraph() {
Just call this CreateGraph()

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode341
src/compiler.h:341: Status OptimizeHGraph() {
OptimizeGraph

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode364
src/compiler.h:364: Status InnerCreateHGraph();
I don't understand why you need these "Inner" methods. Why not just implement
directly?

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode385
src/compiler.h:385: compiler_->total_time_taken_ += (OS::Ticks() - start_);
Perhaps divide this up into three counters, one for each phase so that we can
track the total time taken plus for each phase?

[sanjoy, 2012-07-16 13:46:46]

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode230
src/compiler.cc:230: Timer t(this);
On 2012/07/16 12:19:09, danno wrote:
> nit: Move this down after the comment so diff is clearer.

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode268
src/compiler.cc:268: Handle<Context> global_context(
On 2012/07/16 12:19:09, danno wrote:
> This fits on one line like in the original, no?

No, because I had to replace `info' with `info()'.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode281
src/compiler.cc:281: 
On 2012/07/16 12:19:09, danno wrote:
> Don't add a line here, please keep original white space as much as possible to
> help make diff clearer.

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode281
src/compiler.cc:281: 
On 2012/07/16 12:19:09, danno wrote:
> Don't add a line here, please keep original white space as much as possible to
> help make diff clearer.

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode301
src/compiler.cc:301: if (chunk_ == NULL) {
On 2012/07/16 12:19:09, danno wrote:
> Why will chunk ever be null? It seems like the only path is in the else above,
> why not put the test there and always return SUCCEEDED at the end of this
> funciton?

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode324
src/compiler.cc:324: void OptimizingCompiler::RecordOptimizationStats() {
On 2012/07/16 12:19:09, danno wrote:
> Can you please move this back up to where FinishOptimization used to be? Makes
> the diffs much simpler.

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.cc#newcode352
src/compiler.cc:352: static bool MakeCrankshaftCode(CompilationInfo* info) {
On 2012/07/16 12:19:09, danno wrote:
> Can you please move this up where MakeCrankshaftCode used to be? Again, to
> minimize the diffs if possible.

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode319
src/compiler.h:319: // A helper class that calls the three phases in Crankshaft
and keeps
On 2012/07/16 12:19:09, danno wrote:
> nit: compilation phases
> maybe you should list what they are?

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode337
src/compiler.h:337: Status CreateHGraph() {
On 2012/07/16 12:19:09, danno wrote:
> Just call this CreateGraph()

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode341
src/compiler.h:341: Status OptimizeHGraph() {
On 2012/07/16 12:19:09, danno wrote:
> OptimizeGraph

Done.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode364
src/compiler.h:364: Status InnerCreateHGraph();
On 2012/07/16 12:19:09, danno wrote:
> I don't understand why you need these "Inner" methods. Why not just implement
> directly?

I wanted to have a central place to set last_status_.  I've now added a
SetLastStatus method that also returns the set status so that I can do `return
SetLastStatus(FAILED'.

http://codereview.chromium.org/10700188/diff/2001/src/compiler.h#newcode385
src/compiler.h:385: compiler_->total_time_taken_ += (OS::Ticks() - start_);
On 2012/07/16 12:19:09, danno wrote:
> Perhaps divide this up into three counters, one for each phase so that we can
> track the total time taken plus for each phase?

Done.

[danno, 2012-07-16 15:05:02]

I am a little concerned that you might have subtly changed handlings of
bailouts/failures/inlining. Have you run all of the benchmarks to make sure
there are no regressions?

http://codereview.chromium.org/10700188/diff/10001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10700188/diff/10001/src/compiler.cc#newcode245
src/compiler.cc:245: return AbortOptimization();
Is this really equivalent to what was here before? Before, disableoptimization
was always called. now you only call it in the non-inline case. Is the semantic
the same?

http://codereview.chromium.org/10700188/diff/10001/src/compiler.cc#newcode261
src/compiler.cc:261: return AbortOptimization();
Ditto.

http://codereview.chromium.org/10700188/diff/10001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10700188/diff/10001/src/compiler.h#newcode343
src/compiler.h:343: Status CreateGraph();
MUST_USE_RESULT for this and the next two?

[sanjoy, 2012-07-17 08:09:26]

> I am a little concerned that you might have subtly changed handlings of
> bailouts/failures/inlining. Have you run all of the benchmarks to make sure
> there are no regressions?

That is unlikely -- this code doesn't decide whether a particular
function is to be inlined or not.  Moreover, since the pipeline
basically aborts on a failure / bailout (usually by calling
StackOverflow()) it would be strange if this change modifies the
bailout / failure logic without breaking any tests.

[sanjoy, 2012-07-17 08:09:26]

> I am a little concerned that you might have subtly changed handlings of
> bailouts/failures/inlining. Have you run all of the benchmarks to make sure
> there are no regressions?

That is unlikely -- this code doesn't decide whether a particular
function is to be inlined or not.  Moreover, since the pipeline
basically aborts on a failure / bailout (usually by calling
StackOverflow()) it would be strange if this change modifies the
bailout / failure logic without breaking any tests.

[sanjoy, 2012-07-17 08:09:39]

http://codereview.chromium.org/10700188/diff/10001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10700188/diff/10001/src/compiler.cc#newcode245
src/compiler.cc:245: return AbortOptimization();
On 2012/07/16 15:05:02, danno wrote:
> Is this really equivalent to what was here before? Before, disableoptimization
> was always called. now you only call it in the non-inline case. Is the
semantic
> the same?

Modified this part to make the logic clearer.

http://codereview.chromium.org/10700188/diff/10001/src/compiler.cc#newcode261
src/compiler.cc:261: return AbortOptimization();
On 2012/07/16 15:05:02, danno wrote:
> Ditto.

Ditto.

http://codereview.chromium.org/10700188/diff/10001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10700188/diff/10001/src/compiler.h#newcode343
src/compiler.h:343: Status CreateGraph();
On 2012/07/16 15:05:02, danno wrote:
> MUST_USE_RESULT for this and the next two?

Done.

[danno, 2012-07-17 15:08:38]

LGTM with comments

http://codereview.chromium.org/10700188/diff/11005/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10700188/diff/11005/src/compiler.cc#newcode208
src/compiler.cc:208: return status != OptimizingCompiler::FAILED;
nit: don't return here, or return after every if/else.

Probably better:

OptimizingCompiler::Status status = compiler.CreateGraph();
if (status != OptimizingCompiler::SUCCEEDED) {
  return status != OptimizingCompiler::FAILED;
}

status = nextStep.....
OptimizingCompiler::Status status = compiler.CreateGraph();
if (status != OptimizingCompiler::SUCCEEDED) {
  return status != OptimizingCompiler::FAILED;
}

status = nextSTep....
OptimizingCompiler::Status status = compiler.CreateGraph();
return status != OptimizingCompiler::FAILED;

[sanjoy, 2012-07-17 15:21:35]

Two changes in this version:

 * Addressed review.
 * Added some missing SetLastStatus calls (PTAL)

[danno, 2012-07-17 15:33:30]

lgtm