Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2447993002:
[wasm] Debug-time print for compiler graph (Closed)
|
Created:
4 years, 1 month ago by Mircea Trofin Modified:
4 years, 1 month ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Debug-time print for compiler graph
Simple "Print" API for the compiler graph.
BUG=
Committed: https://crrev.com/e645b560730e0de14002d393385b23b601780028
Cr-Commit-Position: refs/heads/master@{#40571}
Patch Set 1 #
Total comments: 4
Patch Set 2 : AsRPO way #Patch Set 3 : includes #
Messages
Total messages: 26 (19 generated)
Description was changed from ========== [wasm] Debug-time print for compiler graph BUG= ========== to ========== [wasm] Debug-time print for compiler graph Simple "Print" API for the compiler graph. BUG= ==========
mtrofin@chromium.org changed reviewers: + bmeurer@chromium.org, titzer@chromium.org
The CQ bit was checked by mtrofin@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...
mtrofin@chromium.org changed reviewers: + mstarzinger@chromium.org
Hello all, This is an opportunistic CL - I found myself needing this on an unrelated CL, and thought it may come useful to others. I realize we have more complex visualizers. This is meant for scenarios where one works in gdb and tries to get some data interactively.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2447993002/diff/1/src/compiler/graph.cc File src/compiler/graph.cc (left): https://codereview.chromium.org/2447993002/diff/1/src/compiler/graph.cc#oldcode5 src/compiler/graph.cc:5: #include "src/compiler/graph.h" nit: Please keep the graph.h include at the top within the graph.cc file as it was before. This is according to the style guide: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2447993002/diff/1/src/compiler/graph.cc File src/compiler/graph.cc (right): https://codereview.chromium.org/2447993002/diff/1/src/compiler/graph.cc#newco... src/compiler/graph.cc:82: for (auto& pair : map) pair.second->Print(); How would you feel about using "OFStream os(stdout); os << AsRPO(this)" here instead (probably needs to include graph-visualizer.h then)? That way we would use the common format people are already used to from other printout and could avoid having to maintain multiple printing methods.
The CQ bit was checked by mtrofin@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...
https://codereview.chromium.org/2447993002/diff/1/src/compiler/graph.cc File src/compiler/graph.cc (left): https://codereview.chromium.org/2447993002/diff/1/src/compiler/graph.cc#oldcode5 src/compiler/graph.cc:5: #include "src/compiler/graph.h" On 2016/10/25 09:16:03, Michael Starzinger wrote: > nit: Please keep the graph.h include at the top within the graph.cc file as it > was before. This is according to the style guide: > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes Done. https://codereview.chromium.org/2447993002/diff/1/src/compiler/graph.cc File src/compiler/graph.cc (right): https://codereview.chromium.org/2447993002/diff/1/src/compiler/graph.cc#newco... src/compiler/graph.cc:82: for (auto& pair : map) pair.second->Print(); On 2016/10/25 09:16:03, Michael Starzinger wrote: > How would you feel about using "OFStream os(stdout); os << AsRPO(this)" here > instead (probably needs to include graph-visualizer.h then)? That way we would > use the common format people are already used to from other printout and could > avoid having to maintain multiple printing methods. Happy to, and done - it is unfortunate the API isn't more discoverable.
The CQ bit was checked by mtrofin@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...
Patchset #3 (id:30001) has been deleted
The CQ bit was checked by mtrofin@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...
LGTM from my end.
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 mtrofin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [wasm] Debug-time print for compiler graph Simple "Print" API for the compiler graph. BUG= ========== to ========== [wasm] Debug-time print for compiler graph Simple "Print" API for the compiler graph. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:50001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Debug-time print for compiler graph Simple "Print" API for the compiler graph. BUG= ========== to ========== [wasm] Debug-time print for compiler graph Simple "Print" API for the compiler graph. BUG= Committed: https://crrev.com/e645b560730e0de14002d393385b23b601780028 Cr-Commit-Position: refs/heads/master@{#40571} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e645b560730e0de14002d393385b23b601780028 Cr-Commit-Position: refs/heads/master@{#40571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
