Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(5)

Issue 1341613002: Subzero: Fix labels for block profiling. (Closed)

Created:
5 years, 3 months ago by Jim Stichnoth
Modified:
5 years, 3 months ago
Reviewers:
ascull, John
CC:
native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Subzero: Fix labels for block profiling. The problem is that the block profiling pass runs at the very beginning and commits to particular label strings, but the actual label names might change by emission time because of node reordering. There was actually something of a workaround - given a label string from the profile output, inspect the *profiled* asm code and search for the block containing the increment of the counter location, as the name of the counter location label is related to the label string in the profile output. However, it's tedious to mentally filter out the counter update code, and the counter update code has a huge impact on register allocation. The solution is to use a persistent number in CfgNode for constructing the label string, which doesn't change when the nodes are reordered. One note (independent of this change): Without block profiling, empty basic blocks are deleted and don't appear in the asm output. But with block profiling, these blocks are never empty because they contain profile update instructions. This means the profile output may contain labels that don't exist in the non-profiled asm. Another note: New nodes created as a result of edge splitting from advanced phi lowering are not profiled. BUG= none R=ascull@google.com, jpp@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=e7dbc0bcab159a2c64d7d8dff746d273bf80fa36

Patch Set 1 #

Total comments: 2

Patch Set 2 : Code review changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -4 lines) Patch
M src/IceCfg.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M src/IceCfg.cpp View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/IceCfgNode.h View 1 chunk +2 lines, -1 line 0 comments Download
M src/IceCfgNode.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M src/IceClFlags.cpp View 1 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (1 generated)
Jim Stichnoth
5 years, 3 months ago (2015-09-12 18:02:24 UTC) #2
John
lgtm
5 years, 3 months ago (2015-09-14 15:06:51 UTC) #3
ascull
It might be useful to leave a TODO somewhere with the notes you put here ...
5 years, 3 months ago (2015-09-14 16:35:37 UTC) #4
Jim Stichnoth
On 2015/09/14 16:35:37, ascull wrote: > It might be useful to leave a TODO somewhere ...
5 years, 3 months ago (2015-09-15 05:24:03 UTC) #5
Jim Stichnoth
https://codereview.chromium.org/1341613002/diff/1/src/IceCfgNode.cpp File src/IceCfgNode.cpp (right): https://codereview.chromium.org/1341613002/diff/1/src/IceCfgNode.cpp#newcode30 src/IceCfgNode.cpp:30: : Func(Func), Number(LabelNumber), LabelNumber(LabelNumber) {} On 2015/09/14 16:35:37, ascull ...
5 years, 3 months ago (2015-09-15 05:24:09 UTC) #6
ascull
lgtm
5 years, 3 months ago (2015-09-15 16:07:16 UTC) #7
Jim Stichnoth
5 years, 3 months ago (2015-09-15 17:09:28 UTC) #8
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as
e7dbc0bcab159a2c64d7d8dff746d273bf80fa36 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698