Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599433002:
[turbofan]: Better source position information (Closed)
Description[turbofan]: Better source position information
* Ensure that a source position is already specified in generated code before
prologue is assembled.
* Ensure source position is set for instructions before their gaps are assembled
(this fixes missing source position information at the beginning of deferred
code).
* Don't output source position information for gap moves that are
redundant. This led to extraneous, confusing source positions for constants
that did not end up producing any code.
* Output source position information that is usable in turbolizer when --trace-turbo
is specified.
LOG=N
Review-Url: https://codereview.chromium.org/2599433002
Cr-Commit-Position: refs/heads/master@{#41914}
Committed: https://chromium.googlesource.com/v8/v8/+/21dfcf5dadba04c2f2c2146704ab6ea4952f4997
Patch Set 1 #
Total comments: 4
Patch Set 2 : feedback addressed #
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [turbofan]: Better source position information * Ensure that a source position is already specified in generated code before prologue is assembled. * Ensure source position is set for instructions before their gaps are assembled (this fixes missing source position information at the beginning of deferred code). * Don't output source position information for gap moves that are redundant. This led to extraneous, confusing source positions for constants that did not end up producing any code. LOG=N ========== to ========== [turbofan]: Better source position information * Ensure that a source position is already specified in generated code before prologue is assembled. * Ensure source position is set for instructions before their gaps are assembled (this fixes missing source position information at the beginning of deferred code). * Don't output source position information for gap moves that are redundant. This led to extraneous, confusing source positions for constants that did not end up producing any code. * Output source position information that is usable in turbolizer when --trace-turbo is specified. LOG=N ==========
danno@chromium.org changed reviewers: + jarin@chromium.org, tebbi@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator.cc File src/compiler/code-generator.cc (right): https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator... src/compiler/code-generator.cc:502: void CodeGenerator::AssembleSourcePosition(SourcePosition& source_position) { Why not pass by value? It's just a 64bit value. https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator... src/compiler/code-generator.cc:515: << source_position.ScriptOffset() << ">"; Perhaps we should use std::ostream& operator<<(std::ostream& out, const SourcePosition& pos) as defined in source-position.cc here. The format there is slightly more human readable, but could also be adapted. I don't think it is used anywhere.
feedback addressed. please take another look. https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator.cc File src/compiler/code-generator.cc (right): https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator... src/compiler/code-generator.cc:502: void CodeGenerator::AssembleSourcePosition(SourcePosition& source_position) { On 2016/12/21 15:00:55, Tobias Tebbi wrote: > Why not pass by value? It's just a 64bit value. Done. https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator... src/compiler/code-generator.cc:515: << source_position.ScriptOffset() << ">"; On 2016/12/21 15:00:55, Tobias Tebbi wrote: > Perhaps we should use > std::ostream& operator<<(std::ostream& out, const SourcePosition& pos) > as defined in source-position.cc here. The format there is slightly more human > readable, but could also be adapted. I don't think it is used anywhere. Done.
The CQ bit was checked by danno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/21 15:23:14, danno wrote: > feedback addressed. please take another look. > > https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator.cc > File src/compiler/code-generator.cc (right): > > https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator... > src/compiler/code-generator.cc:502: void > CodeGenerator::AssembleSourcePosition(SourcePosition& source_position) { > On 2016/12/21 15:00:55, Tobias Tebbi wrote: > > Why not pass by value? It's just a 64bit value. > > Done. > > https://codereview.chromium.org/2599433002/diff/1/src/compiler/code-generator... > src/compiler/code-generator.cc:515: << source_position.ScriptOffset() << ">"; > On 2016/12/21 15:00:55, Tobias Tebbi wrote: > > Perhaps we should use > > std::ostream& operator<<(std::ostream& out, const SourcePosition& pos) > > as defined in source-position.cc here. The format there is slightly more human > > readable, but could also be adapted. I don't think it is used anywhere. > > Done. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by danno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/31196)
lgtm
The CQ bit was checked by danno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482396236765170,
"parent_rev": "89259657b9d0953ce038a6411f414fd00a8c7a89", "commit_rev":
"21dfcf5dadba04c2f2c2146704ab6ea4952f4997"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan]: Better source position information * Ensure that a source position is already specified in generated code before prologue is assembled. * Ensure source position is set for instructions before their gaps are assembled (this fixes missing source position information at the beginning of deferred code). * Don't output source position information for gap moves that are redundant. This led to extraneous, confusing source positions for constants that did not end up producing any code. * Output source position information that is usable in turbolizer when --trace-turbo is specified. LOG=N ========== to ========== [turbofan]: Better source position information * Ensure that a source position is already specified in generated code before prologue is assembled. * Ensure source position is set for instructions before their gaps are assembled (this fixes missing source position information at the beginning of deferred code). * Don't output source position information for gap moves that are redundant. This led to extraneous, confusing source positions for constants that did not end up producing any code. * Output source position information that is usable in turbolizer when --trace-turbo is specified. LOG=N Review-Url: https://codereview.chromium.org/2599433002 Cr-Commit-Position: refs/heads/master@{#41914} Committed: https://chromium.googlesource.com/v8/v8/+/21dfcf5dadba04c2f2c2146704ab6ea4952... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/21dfcf5dadba04c2f2c2146704ab6ea4952... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
