https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block-profiler.cc
File src/compiler/basic-block-profiler.cc (right):
https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block-profiler.cc#newcode17
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-profiler.cc#newcode40
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-profiler.cc#newcode47
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-profiler.cc#newcode138
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-profiler.h
File src/compiler/basic-block-profiler.h (right):
https://codereview.chromium.org/593563005/diff/90010/src/compiler/basic-block-profiler.h#newcode50
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#newcode14
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.
https://codereview.chromium.org/593563005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
