Doxygenize the documentation comments

Doxygenize the documentation comments

There were many // comment used to document classes, functions etc. but
those are not picked up by doxygen which expects /// comments. This
converts many comments from // to /// in order to improve the generated
documentation.

BUG=
R=jvoung@chromium.org, kschimpf@google.com

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

[ascull, 2015-06-30 18:40:30]

ascull@google.com changed reviewers:
+ jvoung@chromium.org

[jvoung (off chromium), 2015-06-30 22:02:46]

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

[jvoung (off chromium), 2015-06-30 22:04:37]

jvoung@chromium.org changed reviewers:
+ qining@google.com

[jvoung (off chromium), 2015-06-30 22:05:48]

+cc others who would be adding comments

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h#newcode16
src/IceAssembler.h:16: //
LLVM sometimes does "\file" and adds "\\\" to the file-level doc. Does that help
the output?

In LLVM there's only about 38 .h files that do that, out of 713(?!), though...

%git grep "\\\\file" include/ | wc -l                                           
                                      (git)-[master] 
38

 % find include -name "*.h" | wc -l                                             
                                        (git)-[master] 
713

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h#newcode66
src/IceAssembler.h:66: /// To emit an instruction to the assembler buffer, the
EnsureCapacity helper
How about a summary sentence on the first line?
http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

"The first sentence or a paragraph beginning with \brief is used as an
abstract."

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h#newcode124
src/IceAssembler.h:124: /// The member variable is named Assemblr to avoid
hiding the class Assembler.
Probably doesn't need to be publicly documented...

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h#newcode234
src/IceAssembler.h:234: /// Buffer's constructor uses the Allocator, so it needs
to appear after it.
This caveat doesn't need to be doxygenized?

https://codereview.chromium.org/1216963007/diff/1/src/IceAssemblerX8632.h
File src/IceAssemblerX8632.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceAssemblerX8632.h#new...
src/IceAssemblerX8632.h:880: /// The arith_int() methods factor out the
commonality between the encodings of
wrap comment?

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode200
src/IceCfg.h:200: /// Adds a call to the ProfileSummary runtime function as the
first instruction
reflow (past 80 cols)? (fill-paragraph or what the vim-equivalent is)

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode205
src/IceCfg.h:205: /// one of them. It returns a list with all the globals that
the profiling code
reflow

https://codereview.chromium.org/1216963007/diff/1/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceDefs.h#newcode269
src/IceDefs.h:269: /// Options for pooling and randomization of immediates
If you're up for it while you're here, could add a "." at the end =)

https://codereview.chromium.org/1216963007/diff/1/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceGlobalInits.h#newcod...
src/IceGlobalInits.h:327: /// list of initializers for the declared variable.
Capitalized "List of..."

https://codereview.chromium.org/1216963007/diff/1/src/IceIntrinsics.h
File src/IceIntrinsics.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceIntrinsics.h#newcode108
src/IceIntrinsics.h:108: // Basic attributes related to each intrinsic, that are
relevant to
Useful to /// ?

https://codereview.chromium.org/1216963007/diff/1/src/IceTranslator.h
File src/IceTranslator.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceTranslator.h#newcode33
src/IceTranslator.h:33: /// other intermediate representations down to ICE, and
then call the appropriate
reflow

[Karl, 2015-07-06 18:08:49]

I didn't comment on private fields/methods with /// comments. However, my
thought is that (in general) private fields represent internal implementation
detail, and doesn't need to be in doxygen. However, I leave this decision to
others (Jim/Jan).

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode54
src/IceCfg.h:54: /// options at all are set.
Should the next two methods be bundled, since the comment applies to both?

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode58
src/IceCfg.h:58: /// Manage the name and return type of the function being
translated.
Same here.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode63
src/IceCfg.h:63: /// Manage the "internal" attribute of the function.
Same here.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode71
src/IceCfg.h:71: /// translator handle the function.
Same here.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode76
src/IceCfg.h:76: /// Manage nodes (a.k.a. basic blocks, CfgNodes).
Same here.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode89
src/IceCfg.h:89: /// object.
Same here?

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode100
src/IceCfg.h:100: /// Manage instruction numbering.
Same here?

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode106
src/IceCfg.h:106: /// name.  The Node argument is the node where the variable is
defined.
Same here.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode118
src/IceCfg.h:118: // Manage arguments to the function.
Why not here? It is part of the public API.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode125
src/IceCfg.h:125: // Miscellaneous accessors.
Maybe here?

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode168
src/IceCfg.h:168: /// Variable::DefNode field during dumping/emitting.
Again, bundle methods for documentation?

https://codereview.chromium.org/1216963007/diff/1/src/IceCfgNode.h
File src/IceCfgNode.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceCfgNode.h#newcode56
src/IceCfgNode.h:56: // Access predecessor and successor edge lists.
Bundle comment to both methods?

That is use
/// @{
and
/// @}
around the pair of methods (see http://llvm.org/docs/CodingStandards.html).

https://codereview.chromium.org/1216963007/diff/1/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceELFSection.h#newcode44
src/IceELFSection.h:44: // Constructs an ELF section, filling in fields that
will be known
Why not here?

https://codereview.chromium.org/1216963007/diff/1/src/IceELFSection.h#newcode135
src/IceELFSection.h:135: // Model of ELF symbol table entries. Besides keeping
track of the fields
Why not /// here?

https://codereview.chromium.org/1216963007/diff/1/src/IceIntrinsics.h
File src/IceIntrinsics.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceIntrinsics.h#newcode120
src/IceIntrinsics.h:120: IsValidCall,      // Valid use of instrinsic call.
Maybe add /// here to? These comments appear to be defining public API.

https://codereview.chromium.org/1216963007/diff/1/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceOperand.h#newcode69
src/IceOperand.h:69: /// situation where Func==nullptr.
Bundle this comment with next several dump methods?

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:905: // Stack frame layout:
Not clear if this should be a doxygen comment.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1149: /// Change the original ret instruction
into a sandboxed return sequence.
This is in the middle of a method. Should it have doxygen comments?

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1415: /// Limit the number of lea/shl
operations for a single multiply, to
Again, this is within a method. Should it have doxygen comments?

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1772: /// Mask that directs pshufd to create
a vector with entries
Similar question here (not at declaration level).

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:2622: /// Use a helper for x86-32.
Should this be a doxygen comment (inside method).

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:2659: /// i32 on x86-32.
Again, not a declaration. Should it be ///  ?

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3259: ///   T := T[0, 1] ElementR[3, 0]
Again, inside method. Should this be /// ?

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4276: /// registers there either.
Should this be /// (inside method).

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4507: /// registers there either.
Similar here (inside method).

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4957: /// SSE has no left shift operation for
vectors of 8 bit integers.
Why? (inside method).

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4988: /// Compute the location of Loc in
memory.
Why? (inside method).

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5111: /// assigned infinite weight.
Why? (inside method).

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5214: /// registers.  This is for
performance, not correctness.
Why ? (inside method).

[ascull, 2015-07-06 19:29:10]

As this is an aid for developers I think private items are good to document. The
Doxyfile can always be used to remove them if needed and that's a lot easier
than adding all the ///s.

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h#newcode16
src/IceAssembler.h:16: //
On 2015/06/30 22:05:47, jvoung wrote:
> LLVM sometimes does "\file" and adds "\\\" to the file-level doc. Does that
help
> the output?
> 
> In LLVM there's only about 38 .h files that do that, out of 713(?!), though...
> 
> %git grep "\\\\file" include/ | wc -l                                         
 
>                                       (git)-[master] 
> 38
> 
>  % find include -name "*.h" | wc -l                                           
 
>                                         (git)-[master] 
> 713

I've done this naively, just adding \file to all the header comments. \briefs
are probably better written by someone that understands the code better.

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h#newcode66
src/IceAssembler.h:66: /// To emit an instruction to the assembler buffer, the
EnsureCapacity helper
On 2015/06/30 22:05:47, jvoung wrote:
> How about a summary sentence on the first line?
>
http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
> 
> "The first sentence or a paragraph beginning with \brief is used as an
> abstract."

The \brief summaries are probably better written by someone that understands the
code better.

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h#newcode124
src/IceAssembler.h:124: /// The member variable is named Assemblr to avoid
hiding the class Assembler.
On 2015/06/30 22:05:47, jvoung wrote:
> Probably doesn't need to be publicly documented...

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceAssembler.h#newcode234
src/IceAssembler.h:234: /// Buffer's constructor uses the Allocator, so it needs
to appear after it.
On 2015/06/30 22:05:47, jvoung wrote:
> This caveat doesn't need to be doxygenized?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceAssemblerX8632.h
File src/IceAssemblerX8632.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceAssemblerX8632.h#new...
src/IceAssemblerX8632.h:880: /// The arith_int() methods factor out the
commonality between the encodings of
On 2015/06/30 22:05:47, jvoung wrote:
> wrap comment?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode54
src/IceCfg.h:54: /// options at all are set.
On 2015/07/06 18:08:47, Karl wrote:
> Should the next two methods be bundled, since the comment applies to both?

This only applies to isVerbose. It mentions nothing about setVerbose.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode58
src/IceCfg.h:58: /// Manage the name and return type of the function being
translated.
On 2015/07/06 18:08:47, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode63
src/IceCfg.h:63: /// Manage the "internal" attribute of the function.
On 2015/07/06 18:08:48, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode71
src/IceCfg.h:71: /// translator handle the function.
On 2015/07/06 18:08:47, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode76
src/IceCfg.h:76: /// Manage nodes (a.k.a. basic blocks, CfgNodes).
On 2015/07/06 18:08:47, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode89
src/IceCfg.h:89: /// object.
On 2015/07/06 18:08:48, Karl wrote:
> Same here?

Only applies to add.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode100
src/IceCfg.h:100: /// Manage instruction numbering.
On 2015/07/06 18:08:48, Karl wrote:
> Same here?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode106
src/IceCfg.h:106: /// name.  The Node argument is the node where the variable is
defined.
On 2015/07/06 18:08:47, Karl wrote:
> Same here.

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode118
src/IceCfg.h:118: // Manage arguments to the function.
On 2015/07/06 18:08:48, Karl wrote:
> Why not here? It is part of the public API.

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode125
src/IceCfg.h:125: // Miscellaneous accessors.
On 2015/07/06 18:08:47, Karl wrote:
> Maybe here?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode168
src/IceCfg.h:168: /// Variable::DefNode field during dumping/emitting.
On 2015/07/06 18:08:47, Karl wrote:
> Again, bundle methods for documentation?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode200
src/IceCfg.h:200: /// Adds a call to the ProfileSummary runtime function as the
first instruction
On 2015/06/30 22:05:48, jvoung wrote:
> reflow (past 80 cols)? (fill-paragraph or what the vim-equivalent is)

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfg.h#newcode205
src/IceCfg.h:205: /// one of them. It returns a list with all the globals that
the profiling code
On 2015/06/30 22:05:48, jvoung wrote:
> reflow

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceCfgNode.h
File src/IceCfgNode.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceCfgNode.h#newcode56
src/IceCfgNode.h:56: // Access predecessor and successor edge lists.
On 2015/07/06 18:08:48, Karl wrote:
> Bundle comment to both methods?
> 
> That is use
> /// @{
> and
> /// @}
> around the pair of methods (see http://llvm.org/docs/CodingStandards.html).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceDefs.h#newcode269
src/IceDefs.h:269: /// Options for pooling and randomization of immediates
On 2015/06/30 22:05:48, jvoung wrote:
> If you're up for it while you're here, could add a "." at the end =)

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceELFSection.h
File src/IceELFSection.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceELFSection.h#newcode44
src/IceELFSection.h:44: // Constructs an ELF section, filling in fields that
will be known
On 2015/07/06 18:08:48, Karl wrote:
> Why not here?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceELFSection.h#newcode135
src/IceELFSection.h:135: // Model of ELF symbol table entries. Besides keeping
track of the fields
On 2015/07/06 18:08:48, Karl wrote:
> Why not /// here?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceGlobalInits.h#newcod...
src/IceGlobalInits.h:327: /// list of initializers for the declared variable.
On 2015/06/30 22:05:48, jvoung wrote:
> Capitalized "List of..."

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceIntrinsics.h
File src/IceIntrinsics.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceIntrinsics.h#newcode108
src/IceIntrinsics.h:108: // Basic attributes related to each intrinsic, that are
relevant to
On 2015/06/30 22:05:48, jvoung wrote:
> Useful to /// ?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceIntrinsics.h#newcode120
src/IceIntrinsics.h:120: IsValidCall,      // Valid use of instrinsic call.
On 2015/07/06 18:08:48, Karl wrote:
> Maybe add /// here to? These comments appear to be defining public API.

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceOperand.h#newcode69
src/IceOperand.h:69: /// situation where Func==nullptr.
On 2015/07/06 18:08:48, Karl wrote:
> Bundle this comment with next several dump methods?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:905: // Stack frame layout:
On 2015/07/06 18:08:48, Karl wrote:
> Not clear if this should be a doxygen comment.

If it should be documented then it needs to be moved out of the function. Can
someone more experienced with the project decide what to do here?

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1149: /// Change the original ret instruction
into a sandboxed return sequence.
On 2015/07/06 18:08:49, Karl wrote:
> This is in the middle of a method. Should it have doxygen comments?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1415: /// Limit the number of lea/shl
operations for a single multiply, to
On 2015/07/06 18:08:49, Karl wrote:
> Again, this is within a method. Should it have doxygen comments?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:1772: /// Mask that directs pshufd to create
a vector with entries
On 2015/07/06 18:08:48, Karl wrote:
> Similar question here (not at declaration level).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:2622: /// Use a helper for x86-32.
On 2015/07/06 18:08:49, Karl wrote:
> Should this be a doxygen comment (inside method).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:2659: /// i32 on x86-32.
On 2015/07/06 18:08:48, Karl wrote:
> Again, not a declaration. Should it be ///  ?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:3259: ///   T := T[0, 1] ElementR[3, 0]
On 2015/07/06 18:08:49, Karl wrote:
> Again, inside method. Should this be /// ?

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4276: /// registers there either.
On 2015/07/06 18:08:48, Karl wrote:
> Should this be /// (inside method).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4507: /// registers there either.
On 2015/07/06 18:08:48, Karl wrote:
> Similar here (inside method).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4957: /// SSE has no left shift operation for
vectors of 8 bit integers.
On 2015/07/06 18:08:49, Karl wrote:
> Why? (inside method).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4988: /// Compute the location of Loc in
memory.
On 2015/07/06 18:08:49, Karl wrote:
> Why? (inside method).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5111: /// assigned infinite weight.
On 2015/07/06 18:08:49, Karl wrote:
> Why? (inside method).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:5214: /// registers.  This is for
performance, not correctness.
On 2015/07/06 18:08:49, Karl wrote:
> Why ? (inside method).

Done.

https://codereview.chromium.org/1216963007/diff/1/src/IceTranslator.h
File src/IceTranslator.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceTranslator.h#newcode33
src/IceTranslator.h:33: /// other intermediate representations down to ICE, and
then call the appropriate
On 2015/06/30 22:05:48, jvoung wrote:
> reflow

Done.

[jvoung (off chromium), 2015-07-06 20:07:44]

Thanks Andrew!

(and Karl for the second review)

otherwise LGTM

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1216963007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:905: // Stack frame layout:
On 2015/07/06 19:29:09, ascull wrote:
> On 2015/07/06 18:08:48, Karl wrote:
> > Not clear if this should be a doxygen comment.
> 
> If it should be documented then it needs to be moved out of the function. Can
> someone more experienced with the project decide what to do here?

Let's not doxygenize it for now.

This is explaining how addProlog will lay out the stack, which is mostly a
concern for addProlog. Some of the other code cares about some aspects of the
stack layout (proper alignment, alloca comes last), but necessarily the ordering
(e.g., multi-block/global comes first vs single-block/local).

https://codereview.chromium.org/1216963007/diff/20001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1216963007/diff/20001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:12: /// It can either directly parse the binary bitcode
file, or use LLVM routines to
reflow? (80 col)

[Karl, 2015-07-06 20:08:53]

lgtm

[ascull, 2015-07-06 21:24:55]

Final fix for comment reflow

https://codereview.chromium.org/1216963007/diff/20001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1216963007/diff/20001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:12: /// It can either directly parse the binary bitcode
file, or use LLVM routines to
On 2015/07/06 20:07:43, jvoung wrote:
> reflow? (80 col)

Done.

[jvoung (off chromium), 2015-07-06 21:43:25]

https://codereview.chromium.org/1216963007/diff/40001/src/IceCompiler.cpp
File src/IceCompiler.cpp (right):

https://codereview.chromium.org/1216963007/diff/40001/src/IceCompiler.cpp#new...
src/IceCompiler.cpp:12: /// It can either directly parse the binary bitcode
file, or use LLVM routines longer than needed
"longer than needed"?

[ascull, 2015-07-06 21:52:55]

Remove the typo in comments and rebase onto master

[ascull, 2015-07-06 21:53:30]

Committed patchset #4 (id:60001) manually as
9612d32c7e5eb2cb403686ef31172d42e075e460 (presubmit successful).