[turbofan] basic block profiler

[turbofan] basic block profiler

R=titzer@chromium.org, mstarzinger@chromium.org

BUG=

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

[dcarney, 2014-09-23 12:32:38]

dcarney@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[dcarney, 2014-09-23 12:33:03]

ptal
 i still need to add unit tests

[dcarney, 2014-09-23 13:51:19]

dcarney@chromium.org changed reviewers:
+ titzer@chromium.org

[dcarney, 2014-09-23 13:51:52]

Okay, ben can you take a look?  I'm working on unit tests now.

[titzer, 2014-09-23 14:14:09]

https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block...
File src/compiler/basic-block-profiler.cc (right):

https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block...
src/compiler/basic-block-profiler.cc:17: BasicBlockProfiler::Data::Data(int
n_blocks, int* block_ids, uint32_t* counts,
Can you make the constructor allocate the block_ids and counts arrays? Thus the
ownership of those is entirely in this class.

https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block...
src/compiler/basic-block-profiler.cc:40: // TODO(dcarney): reenable this once
code printing is fixed.
What's broken with code printing?

https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block...
src/compiler/basic-block-profiler.cc:47: void
BasicBlockProfiler::Data::DumpProfilingData() {
Can you use OFStream here? It seems to be the way the winds are blowing.

https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block...
src/compiler/basic-block-profiler.cc:138: block_ids[block_number] = block->id();
Pull the body of this loop out into its own function. BTW you shouldn't have to
bother caching the load and store operators now. They are cached in the operator
builder now.

https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block...
File src/compiler/basic-block-profiler.h (right):

https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block...
src/compiler/basic-block-profiler.h:50: Data* Profile(CompilationInfo* info,
Graph* graph, Schedule* schedule);
I think "Instrument" is a better name here.

https://codereview.chromium.org/593563005/diff/90010/src/compiler/pipeline.h
File src/compiler/pipeline.h (right):

https://codereview.chromium.org/593563005/diff/90010/src/compiler/pipeline.h#...
src/compiler/pipeline.h:14: #include "src/compiler/basic-block-profiler.h"
This ones a no go. Please fix.

https://codereview.chromium.org/593563005/diff/90010/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/593563005/diff/90010/src/isolate.h#newcode91
src/isolate.h:91: namespace compiler {
The more I think about it, this thing can't (entirely) live in the compiler
directory, i.e. V8 cannot depend on TurboFan internals. So you will have to
split the BasicBlockProfiler part that does the instrumentation to live in
TurboFan, and the other part live in V8.

[dcarney, 2014-09-24 14:01:15]

Patchset #14 (id:250001) has been deleted

[dcarney, 2014-09-24 14:42:16]

everything addressed - ptal
starting to add unit tests

[titzer, 2014-09-24 14:59:31]

https://codereview.chromium.org/593563005/diff/290001/src/basic-block-profiler.h
File src/basic-block-profiler.h (right):

https://codereview.chromium.org/593563005/diff/290001/src/basic-block-profile...
src/basic-block-profiler.h:26: int* block_ids() { return &block_ids_[0]; }
set_block_id(int block, int id) and hide it?

https://codereview.chromium.org/593563005/diff/290001/src/compiler/basic-bloc...
File src/compiler/basic-block-instrumentor.cc (right):

https://codereview.chromium.org/593563005/diff/290001/src/compiler/basic-bloc...
src/compiler/basic-block-instrumentor.cc:16: static NodeVector::iterator
FindInsertionPoint(NodeVector* nodes) {
Comment me.

https://codereview.chromium.org/593563005/diff/290001/src/compiler/basic-bloc...
src/compiler/basic-block-instrumentor.cc:72: BasicBlock* block = (*it);
Pull me out into a separate function

https://codereview.chromium.org/593563005/diff/290001/src/compiler/basic-bloc...
src/compiler/basic-block-instrumentor.cc:82: Node* load =
graph->NewNode(machine.Load(kMachUint32), base, index);
I'd just smash in the direct pointer into the middle here, so we can have that
as the base and always index 0.

https://codereview.chromium.org/593563005/diff/290001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/593563005/diff/290001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:419: code->Disassemble(NULL, os);
Why do we need to eagerly disassemble the code?

https://codereview.chromium.org/593563005/diff/290001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/593563005/diff/290001/src/flag-definitions.h#...
src/flag-definitions.h:353: DEFINE_STRING(turbo_profiling_filter, "*",
"profiling filter in TurboFan")
Let's just profile it all...

https://codereview.chromium.org/593563005/diff/290001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/593563005/diff/290001/tools/gyp/v8.gyp#newcod...
tools/gyp/v8.gyp:355: '../../src/basic-block-profiler.h',
Don't forget build.gn.

[dcarney, 2014-09-25 08:15:15]

https://codereview.chromium.org/593563005/diff/290001/src/basic-block-profiler.h
File src/basic-block-profiler.h (right):

https://codereview.chromium.org/593563005/diff/290001/src/basic-block-profile...
src/basic-block-profiler.h:26: int* block_ids() { return &block_ids_[0]; }
On 2014/09/24 14:59:31, titzer wrote:
> set_block_id(int block, int id) and hide it?

Done.

https://codereview.chromium.org/593563005/diff/290001/src/compiler/basic-bloc...
File src/compiler/basic-block-instrumentor.cc (right):

https://codereview.chromium.org/593563005/diff/290001/src/compiler/basic-bloc...
src/compiler/basic-block-instrumentor.cc:16: static NodeVector::iterator
FindInsertionPoint(NodeVector* nodes) {
On 2014/09/24 14:59:31, titzer wrote:
> Comment me.

Done.

https://codereview.chromium.org/593563005/diff/290001/src/compiler/basic-bloc...
src/compiler/basic-block-instrumentor.cc:72: BasicBlock* block = (*it);
On 2014/09/24 14:59:31, titzer wrote:
> Pull me out into a separate function

that would be ugly

https://codereview.chromium.org/593563005/diff/290001/src/compiler/basic-bloc...
src/compiler/basic-block-instrumentor.cc:82: Node* load =
graph->NewNode(machine.Load(kMachUint32), base, index);
On 2014/09/24 14:59:31, titzer wrote:
> I'd just smash in the direct pointer into the middle here, so we can have that
> as the base and always index 0.

Done.

https://codereview.chromium.org/593563005/diff/290001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/593563005/diff/290001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:419: code->Disassemble(NULL, os);
On 2014/09/24 14:59:31, titzer wrote:
> Why do we need to eagerly disassemble the code?

there's no heap pointers kept alive in the profiler, so a code dump here is
needed for later output.

https://codereview.chromium.org/593563005/diff/290001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/593563005/diff/290001/src/flag-definitions.h#...
src/flag-definitions.h:353: DEFINE_STRING(turbo_profiling_filter, "*",
"profiling filter in TurboFan")
On 2014/09/24 14:59:31, titzer wrote:
> Let's just profile it all...

okay, but I've found it useful to limit the output

https://codereview.chromium.org/593563005/diff/290001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/593563005/diff/290001/tools/gyp/v8.gyp#newcod...
tools/gyp/v8.gyp:355: '../../src/basic-block-profiler.h',
On 2014/09/24 14:59:31, titzer wrote:
> Don't forget build.gn.

Done.

[titzer, 2014-09-25 09:13:59]

lgtm with comment

https://codereview.chromium.org/593563005/diff/290001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/593563005/diff/290001/src/flag-definitions.h#...
src/flag-definitions.h:353: DEFINE_STRING(turbo_profiling_filter, "*",
"profiling filter in TurboFan")
On 2014/09/25 08:15:15, dcarney wrote:
> On 2014/09/24 14:59:31, titzer wrote:
> > Let's just profile it all...
> 
> okay, but I've found it useful to limit the output

Ok, you can leave it in.

https://codereview.chromium.org/593563005/diff/330001/src/compiler/basic-bloc...
File src/compiler/basic-block-instrumentor.cc (right):

https://codereview.chromium.org/593563005/diff/330001/src/compiler/basic-bloc...
src/compiler/basic-block-instrumentor.cc:87: Node* to_insert[kArraySize] =
{zero, one, base, load, inc, store};
I don't think you want to insert zero and one in every block; every node should
have just one place in the schedule.

[dcarney, 2014-09-25 09:17:11]

https://codereview.chromium.org/593563005/diff/330001/src/compiler/basic-bloc...
File src/compiler/basic-block-instrumentor.cc (right):

https://codereview.chromium.org/593563005/diff/330001/src/compiler/basic-bloc...
src/compiler/basic-block-instrumentor.cc:87: Node* to_insert[kArraySize] =
{zero, one, base, load, inc, store};
On 2014/09/25 09:13:59, titzer wrote:
> I don't think you want to insert zero and one in every block; every node
should
> have just one place in the schedule.

zero and one ARE in only one spot, see next line

[titzer, 2014-09-26 10:45:05]

On 2014/09/25 09:17:11, dcarney wrote:
>
https://codereview.chromium.org/593563005/diff/330001/src/compiler/basic-bloc...
> File src/compiler/basic-block-instrumentor.cc (right):
> 
>
https://codereview.chromium.org/593563005/diff/330001/src/compiler/basic-bloc...
> src/compiler/basic-block-instrumentor.cc:87: Node* to_insert[kArraySize] =
> {zero, one, base, load, inc, store};
> On 2014/09/25 09:13:59, titzer wrote:
> > I don't think you want to insert zero and one in every block; every node
> should
> > have just one place in the schedule.
> 
> zero and one ARE in only one spot, see next line

Land me!

[dcarney, 2014-09-29 07:29:31]

Committed patchset #19 (id:370001) manually as 24263 (presubmit successful).