Subzero: Basic Block Profiler.

Subzero: Basic Block Profiler.

BUG= None
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=f8b4cc8462abb12773f47ddb7ad6fe5f60aee4b6

[John, 2015-06-08 21:32:56]

jpp@chromium.org changed reviewers:
+ jvoung@chromium.org, kschimpf@google.com, mtrofin@chromium.org,
stichnot@chromium.org

[John, 2015-06-08 21:33:17]

Some comments.

https://codereview.chromium.org/1147023007/diff/80001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1147023007/diff/80001/Makefile.standalone#new...
Makefile.standalone:288: runtime.is.built: $(RT_SRC) pydir/build-runtime.py
I found it useful to have pydir/build-runtime.py as a dependency for szrt.o Glad
to remove.

https://codereview.chromium.org/1147023007/diff/80001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (left):

https://codereview.chromium.org/1147023007/diff/80001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:386: const auto Data =
Early in the implementation I was touching this file, and I ran clang-format on
it. Glad to revert.

https://codereview.chromium.org/1147023007/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (left):

https://codereview.chromium.org/1147023007/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:3123: Instr->getDest(),
Early in the implementation I was touching this file, and I ran clang-format on
it. Glad to revert.

[Jim Stichnoth, 2015-06-08 23:45:32]

https://codereview.chromium.org/1147023007/diff/80001/Makefile.standalone
File Makefile.standalone (right):

https://codereview.chromium.org/1147023007/diff/80001/Makefile.standalone#new...
Makefile.standalone:288: runtime.is.built: $(RT_SRC) pydir/build-runtime.py
On 2015/06/08 21:33:16, John wrote:
> I found it useful to have pydir/build-runtime.py as a dependency for szrt.o
Glad
> to remove.

Seems harmless and actually useful this way...

https://codereview.chromium.org/1147023007/diff/80001/pydir/build-runtime.py
File pydir/build-runtime.py (right):

https://codereview.chromium.org/1147023007/diff/80001/pydir/build-runtime.py#...
pydir/build-runtime.py:26: '--strip-symbol=nacl_tp_tdb_offset',
cool :)

https://codereview.chromium.org/1147023007/diff/80001/pydir/build-runtime.py#...
pydir/build-runtime.py:83: # Translate tempdir/szrt.ll and tempdir/szrt_ll.ll to
szrt_native_x8632.tmp.o
80-col, here and below

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py
File pydir/szbuild.py (right):

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py#newcode88
pydir/szbuild.py:88: argparser.add_argument('--enable-block-profile',
dest='enable_block_profile',
80-col

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py#newcode90
pydir/szbuild.py:90: help='Enabled Basic Block profiling.')
Enabled ==> Enable
and lowercase "basic block"

Also, can you fix "Enabled sandboxing" similarly? :)

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py#newcod...
pydir/szbuild.py:223: (['-enable-block-profile'] if args.enable_block_profile
and not args.sandbox else []) +
80-col

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py#newcod...
pydir/szbuild.py:314:
'{root}/toolchain/linux_x86/pnacl_newlib/translator/x86-32-linux/' +
Please keep pnacl_newlib_raw instead of pnacl_newlib

https://codereview.chromium.org/1147023007/diff/80001/runtime/szrt_profiler.c
File runtime/szrt_profiler.c (right):

https://codereview.chromium.org/1147023007/diff/80001/runtime/szrt_profiler.c...
runtime/szrt_profiler.c:9: extern const struct BlockProfileInfo
*block_profile_info;
For this and profile_summary, you may want to try to avoid name conflicts (not
sure if the name conflict possibility goes away in a pexe where the symbols are
almost all internal).

Maybe use __Sz as a prefix, like the other parts of the runtime?

https://codereview.chromium.org/1147023007/diff/80001/runtime/szrt_profiler.c...
runtime/szrt_profiler.c:11: const char SubzeroLogo[] =
static?

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp#newcode81
src/IceCfg.cpp:81: constexpr char BlockStatsGlobalPrefix[] = ".Lprofile_";
Document somewhere that this is a "reserved" prefix?

Also, I think there would be less chance of label name collisions if reserved
prefixes begin with ".L$".  So, ".L$block_name_" and ".L$profile_".

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp#newcode105
src/IceCfg.cpp:105: } // end of namespace
end of anonymous namespace

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp#newcode107
src/IceCfg.cpp:107: std::unique_ptr<VariableDeclarationList>
Cfg::profileBlocks() {
Add light documentation of these two functions, either here or in the .h file.

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:1250: auto *Context = Func->getContext();
In Subzero, we've been more sparing with our use of "auto".  I think the
generalization is that auto is OK if:

- the type is redundant, e.g. "auto *V = llvm::dyn_cast<Variable>(O);"

- there is otherwise some horribly verbose type like with iterators

- the type is blindingly obvious or available a few lines away

For the cluster starting with RMWI64Name, I would use Operand* .

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:161: cl::opt<bool> EnableBlockProfile(
This should probably go between DumpStats and FunctionSections (even though
apparently name changes and neglect have made some items creep out of order).

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:163: cl::desc("If true, instruments basic blocks, and output
profiling "
"instrument" instead of "instruments", for consistency with other help strings
(and also for self-consistency)

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:164: "information at the end of program execution."),
Mention that it outputs to stdout?

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:279: OutFlags.EnableBlockProfile = false;
move this between DumpStats and FunctionSections

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.h#newcod...
src/IceClFlags.h:103: bool getEnableBlockProfile() const { return
EnableBlockProfile; }
Sort these alphabetically within the bool section.  Same for the field
declaration below.

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlagsExtra.h
File src/IceClFlagsExtra.h (left):

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlagsExtra.h#o...
src/IceClFlagsExtra.h:43: 
Restore blank line?

https://codereview.chromium.org/1147023007/diff/80001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (left):

https://codereview.chromium.org/1147023007/diff/80001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:386: const auto Data =
On 2015/06/08 21:33:16, John wrote:
> Early in the implementation I was touching this file, and I ran clang-format
on
> it. Glad to revert.

Just make sure you've checked out and built the latest llvm master, also that
you're rebased to the latest subzero master, and run "make -f
Makefile.standalone format".

We're in this llvm 3.6==>3.7 transition period which makes it more likely to
have formatting wars like this.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:263: constexpr bool HasTailCall = false;
We've been using "const bool ..." instead of constexpr in the rest of the code
base.  Wondering if there is any special benefit to using constexpr on a
primitive type?

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:268: } // end of namespace
end of anonymous namespace

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:293: // ProfilerInits contains the VariableDeclarations
for all the
Would it be reasonable to move this new code into Cfg::translate()?  in the
interest of trying to limit translateFunctions() to work queue management

I guess this would mean that Cfg needs to hold ProfileInits, but that seems in
line with other Cfg responsibilities.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:299: if (matchSymbolName(Func->getFunctionName(),
"exit")) {
Does a normal return from main() lead to a call to exit() ?

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:352: Var->addInitializer(new
VariableDeclaration::RelocInitializer(Global, 0));
Can you "document" the 0, and the 8 a few lines down, by passing in a const
whatever value that is initialized to 0/8?

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:362: // Purposefully create the Var temp to prevent
bugs in case the compilers
compilers ==> compiler

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:386: // is disabled. In practice, given that
szrt_profiler.o will always be embeded
embedded

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:400: void
AddAllIfNotNull(std::unique_ptr<VariableDeclarationList> src,
AddAllIfNotNull ==> addAllIfNotNull

https://codereview.chromium.org/1147023007/diff/80001/src/IceThreading.h
File src/IceThreading.h (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceThreading.h#newc...
src/IceThreading.h:207: // ProfilerInits holds the initializers for an
ProfilerInfo[].
"for a" ?

[John, 2015-06-09 15:36:20]

Ready for second pass.

https://codereview.chromium.org/1147023007/diff/80001/pydir/build-runtime.py
File pydir/build-runtime.py (right):

https://codereview.chromium.org/1147023007/diff/80001/pydir/build-runtime.py#...
pydir/build-runtime.py:83: # Translate tempdir/szrt.ll and tempdir/szrt_ll.ll to
szrt_native_x8632.tmp.o
On 2015/06/08 23:45:30, stichnot wrote:
> 80-col, here and below

Done.

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py
File pydir/szbuild.py (right):

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py#newcode88
pydir/szbuild.py:88: argparser.add_argument('--enable-block-profile',
dest='enable_block_profile',
On 2015/06/08 23:45:31, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py#newcode90
pydir/szbuild.py:90: help='Enabled Basic Block profiling.')
On 2015/06/08 23:45:30, stichnot wrote:
> Enabled ==> Enable
> and lowercase "basic block"
> 
> Also, can you fix "Enabled sandboxing" similarly? :)

Done.

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py#newcod...
pydir/szbuild.py:223: (['-enable-block-profile'] if args.enable_block_profile
and not args.sandbox else []) +
On 2015/06/08 23:45:30, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/1147023007/diff/80001/pydir/szbuild.py#newcod...
pydir/szbuild.py:314:
'{root}/toolchain/linux_x86/pnacl_newlib/translator/x86-32-linux/' +
On 2015/06/08 23:45:31, stichnot wrote:
> Please keep pnacl_newlib_raw instead of pnacl_newlib

Done.

https://codereview.chromium.org/1147023007/diff/80001/runtime/szrt_profiler.c
File runtime/szrt_profiler.c (right):

https://codereview.chromium.org/1147023007/diff/80001/runtime/szrt_profiler.c...
runtime/szrt_profiler.c:9: extern const struct BlockProfileInfo
*block_profile_info;
On 2015/06/08 23:45:31, stichnot wrote:
> For this and profile_summary, you may want to try to avoid name conflicts (not
> sure if the name conflict possibility goes away in a pexe where the symbols
are
> almost all internal).
> 
> Maybe use __Sz as a prefix, like the other parts of the runtime?

Done.

https://codereview.chromium.org/1147023007/diff/80001/runtime/szrt_profiler.c...
runtime/szrt_profiler.c:11: const char SubzeroLogo[] =
On 2015/06/08 23:45:31, stichnot wrote:
> static?

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp#newcode81
src/IceCfg.cpp:81: constexpr char BlockStatsGlobalPrefix[] = ".Lprofile_";
On 2015/06/08 23:45:31, stichnot wrote:
> Document somewhere that this is a "reserved" prefix?
> 
> Also, I think there would be less chance of label name collisions if reserved
> prefixes begin with ".L$".  So, ".L$block_name_" and ".L$profile_".

What do you mean, "reserved"?

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp#newcode105
src/IceCfg.cpp:105: } // end of namespace
On 2015/06/08 23:45:31, stichnot wrote:
> end of anonymous namespace

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp#newcode107
src/IceCfg.cpp:107: std::unique_ptr<VariableDeclarationList>
Cfg::profileBlocks() {
On 2015/06/08 23:45:31, stichnot wrote:
> Add light documentation of these two functions, either here or in the .h file.

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:1250: auto *Context = Func->getContext();
On 2015/06/08 23:45:31, stichnot wrote:
> In Subzero, we've been more sparing with our use of "auto".  I think the
> generalization is that auto is OK if:
> 
> - the type is redundant, e.g. "auto *V = llvm::dyn_cast<Variable>(O);"
> 
> - there is otherwise some horribly verbose type like with iterators
> 
> - the type is blindingly obvious or available a few lines away
> 
> For the cluster starting with RMWI64Name, I would use Operand* .

Done. I was just following items 5 and 6 from Modern Effective C++: "Prefer auto
to explicit type declarations," and "Use the explicit type initializer idiom
when auto deduces undesired types."

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:161: cl::opt<bool> EnableBlockProfile(
On 2015/06/08 23:45:31, stichnot wrote:
> This should probably go between DumpStats and FunctionSections (even though
> apparently name changes and neglect have made some items creep out of order).

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:163: cl::desc("If true, instruments basic blocks, and output
profiling "
On 2015/06/08 23:45:31, stichnot wrote:
> "instrument" instead of "instruments", for consistency with other help strings
> (and also for self-consistency)

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:164: "information at the end of program execution."),
On 2015/06/08 23:45:31, stichnot wrote:
> Mention that it outputs to stdout?

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:279: OutFlags.EnableBlockProfile = false;
On 2015/06/08 23:45:31, stichnot wrote:
> move this between DumpStats and FunctionSections

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlags.h#newcod...
src/IceClFlags.h:103: bool getEnableBlockProfile() const { return
EnableBlockProfile; }
On 2015/06/08 23:45:31, stichnot wrote:
> Sort these alphabetically within the bool section.  Same for the field
> declaration below.

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlagsExtra.h
File src/IceClFlagsExtra.h (left):

https://codereview.chromium.org/1147023007/diff/80001/src/IceClFlagsExtra.h#o...
src/IceClFlagsExtra.h:43: 
On 2015/06/08 23:45:31, stichnot wrote:
> Restore blank line?

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:263: constexpr bool HasTailCall = false;
On 2015/06/08 23:45:32, stichnot wrote:
> We've been using "const bool ..." instead of constexpr in the rest of the code
> base.  Wondering if there is any special benefit to using constexpr on a
> primitive type?

constexpr is a compile time constant, and the compiler should be able to
propagate the value where it is used. const is a constant variable, not
necessarily a constant. Happy to change.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:268: } // end of namespace
On 2015/06/08 23:45:31, stichnot wrote:
> end of anonymous namespace

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:293: // ProfilerInits contains the VariableDeclarations
for all the
On 2015/06/08 23:45:32, stichnot wrote:
> Would it be reasonable to move this new code into Cfg::translate()?  in the
> interest of trying to limit translateFunctions() to work queue management
> 
> I guess this would mean that Cfg needs to hold ProfileInits, but that seems in
> line with other Cfg responsibilities.

I thought about that in the beginning, but then I decided to go with the current
design. No strong opinions. Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:299: if (matchSymbolName(Func->getFunctionName(),
"exit")) {
On 2015/06/08 23:45:32, stichnot wrote:
> Does a normal return from main() lead to a call to exit() ?

It does, at least in the current library:

        subl    $16, %esp
        movl    %edi, (%esp)    # END=%edi
        movl    %ebx, 4(%esp)   # END=%ebx
        call    main
        addl    $16, %esp


        subl    $16, %esp
        movl    %eax, (%esp)    # END=%eax,%esp
        call    exit

        ud2
                                # LiveOut=

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:352: Var->addInitializer(new
VariableDeclaration::RelocInitializer(Global, 0));
On 2015/06/08 23:45:32, stichnot wrote:
> Can you "document" the 0, and the 8 a few lines down, by passing in a const
> whatever value that is initialized to 0/8?

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:362: // Purposefully create the Var temp to prevent
bugs in case the compilers
On 2015/06/08 23:45:32, stichnot wrote:
> compilers ==> compiler

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:386: // is disabled. In practice, given that
szrt_profiler.o will always be embeded
On 2015/06/08 23:45:31, stichnot wrote:
> embedded

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:400: void
AddAllIfNotNull(std::unique_ptr<VariableDeclarationList> src,
On 2015/06/08 23:45:32, stichnot wrote:
> AddAllIfNotNull ==> addAllIfNotNull

Done.

https://codereview.chromium.org/1147023007/diff/80001/src/IceThreading.h
File src/IceThreading.h (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceThreading.h#newc...
src/IceThreading.h:207: // ProfilerInits holds the initializers for an
ProfilerInfo[].
On 2015/06/08 23:45:32, stichnot wrote:
> "for a" ?

No longer relevant. I add the globals to GlobalInits.

[Jim Stichnoth, 2015-06-09 16:39:46]

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp#newcode81
src/IceCfg.cpp:81: constexpr char BlockStatsGlobalPrefix[] = ".Lprofile_";
On 2015/06/09 15:36:18, John wrote:
> On 2015/06/08 23:45:31, stichnot wrote:
> > Document somewhere that this is a "reserved" prefix?
> > 
> > Also, I think there would be less chance of label name collisions if
reserved
> > prefixes begin with ".L$".  So, ".L$block_name_" and ".L$profile_".
> 
> What do you mean, "reserved"?

For a function named "foo", Subzero generates internal label strings as
  ".L" + "foo" + "$" + nodename
  ".L" + "foo" + "$local$__" + label_number
The first is for basic block labels, and the second is for intra-block branch
labels.

My concern here whether some bitcode could be conjured up where one of these
generated labels is the same as a generated profiling label, causing
-filetype=asm output to break, and possibly -filetype=obj as well.

I think we can count on function names not containing '$' (though not
necessarily node names), so labels starting with ".L$" should have no chance of
conflicting.  In that case, I think ".L$block_name_" and ".L$profile_" should be
good prefixes.  I would avoid the __Sz part unless it is to be an externally
visible symbol such as in the Subzero runtime, but in that case the __Sz should
be at the beginning of the symbol name, rather than the middle.

With all that, never mind the documentation request, maybe just tweak the prefix
strings.

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:1250: auto *Context = Func->getContext();
On 2015/06/09 15:36:18, John wrote:
> On 2015/06/08 23:45:31, stichnot wrote:
> > In Subzero, we've been more sparing with our use of "auto".  I think the
> > generalization is that auto is OK if:
> > 
> > - the type is redundant, e.g. "auto *V = llvm::dyn_cast<Variable>(O);"
> > 
> > - there is otherwise some horribly verbose type like with iterators
> > 
> > - the type is blindingly obvious or available a few lines away
> > 
> > For the cluster starting with RMWI64Name, I would use Operand* .
> 
> Done. I was just following items 5 and 6 from Modern Effective C++: "Prefer
auto
> to explicit type declarations," and "Use the explicit type initializer idiom
> when auto deduces undesired types."

I'm fairly new to "auto" and as such, the code base has many inconsistencies and
needs to have a separate cleanup pass, assuming we settle on an "official"
policy.

When we switched over to C++11, I went a bit auto-crazy, and then pulled back a
bit based on JF's (fairly strong) opinions on the matter...

https://codereview.chromium.org/1147023007/diff/120001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1147023007/diff/120001/src/IceGlobalContext.c...
src/IceGlobalContext.cpp:340: constexpr SizeT ByteCountOf64BitNullPtr = 8;
I think the 8 could be typeWidthInBytes(IceType_i64), right?

I thought this would be "byte count of primitive 64-bit integer", not "byte
count of some pointer", especially since all PNaCl pointers are 32-bits
regardless of architecture.

Should the setAlignment(8) above also use typeWidthInBytes(IceType_i64)?

[John, 2015-06-09 22:01:16]

Ready for third pass.

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/1147023007/diff/80001/src/IceCfg.cpp#newcode81
src/IceCfg.cpp:81: constexpr char BlockStatsGlobalPrefix[] = ".Lprofile_";
On 2015/06/09 16:39:45, stichnot wrote:
> On 2015/06/09 15:36:18, John wrote:
> > On 2015/06/08 23:45:31, stichnot wrote:
> > > Document somewhere that this is a "reserved" prefix?
> > > 
> > > Also, I think there would be less chance of label name collisions if
> reserved
> > > prefixes begin with ".L$".  So, ".L$block_name_" and ".L$profile_".
> > 
> > What do you mean, "reserved"?
> 
> For a function named "foo", Subzero generates internal label strings as
>   ".L" + "foo" + "$" + nodename
>   ".L" + "foo" + "$local$__" + label_number
> The first is for basic block labels, and the second is for intra-block branch
> labels.
> 
> My concern here whether some bitcode could be conjured up where one of these
> generated labels is the same as a generated profiling label, causing
> -filetype=asm output to break, and possibly -filetype=obj as well.
> 
> I think we can count on function names not containing '$' (though not
> necessarily node names), so labels starting with ".L$" should have no chance
of
> conflicting.  In that case, I think ".L$block_name_" and ".L$profile_" should
be
> good prefixes.  I would avoid the __Sz part unless it is to be an externally
> visible symbol such as in the Subzero runtime, but in that case the __Sz
should
> be at the beginning of the symbol name, rather than the middle.
> 
> With all that, never mind the documentation request, maybe just tweak the
prefix
> strings.

To avoid name clashes (as long as $ is not a valid identifier, that is) I
changed the prefixes to

.L$profiler$block_name$ and
.L$profiler$block_info$ and

https://codereview.chromium.org/1147023007/diff/120001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1147023007/diff/120001/src/IceGlobalContext.c...
src/IceGlobalContext.cpp:340: constexpr SizeT ByteCountOf64BitNullPtr = 8;
On 2015/06/09 16:39:46, stichnot wrote:
> I think the 8 could be typeWidthInBytes(IceType_i64), right?
> 
> I thought this would be "byte count of primitive 64-bit integer", not "byte
> count of some pointer", especially since all PNaCl pointers are 32-bits
> regardless of architecture.
> 
> Should the setAlignment(8) above also use typeWidthInBytes(IceType_i64)?

Done.

[Jim Stichnoth, 2015-06-09 23:10:49]

lgtm

[John, 2015-06-10 01:06:24]

Committed patchset #8 (id:140001) manually as
f8b4cc8462abb12773f47ddb7ad6fe5f60aee4b6 (presubmit successful).