Issue 266713003: Initial draft of PNaCl bitcode files document.

Karl

Here is my initial (incomplete) draft for the documentation for PNaCl bitcode files. I'm looking ...

6 years, 7 months ago (2014-05-01 17:43:31 UTC) #1

native-client-reviews_googlegroups.com

Raising it in your respective inboxes. On Thu, May 1, 2014 at 10:43 AM, <kschimpf@google.com> ...

6 years, 7 months ago (2014-05-02 17:18:02 UTC) #2

chromium-reviews

Raising it in your respective inboxes. On Thu, May 1, 2014 at 10:43 AM, <kschimpf@google.com> ...

6 years, 7 months ago (2014-05-02 17:18:03 UTC) #3

native-client-reviews_googlegroups.com

I patched in this CL and am serving it from my desktop here: http://ennay2.mtv.corp.google.com:8000/native-client/reference/pnacl-bitcode-manual On ...

6 years, 7 months ago (2014-05-02 17:26:17 UTC) #4

I patched in this CL and am serving it from my desktop here:
http://ennay2.mtv.corp.google.com:8000/native-client/reference/pnacl-bitcode-...


On Fri, May 2, 2014 at 10:18 AM, David Sehr <sehr@google.com> wrote:

> Raising it in your respective inboxes.
>
>
> On Thu, May 1, 2014 at 10:43 AM, <kschimpf@google.com> wrote:
>
>> Reviewers: nacl-eng_google.com,
>>
>> Message:
>> Here is my initial (incomplete) draft for the documentation for PNaCl
>> bitcode
>> files. I'm looking for feedback on style etc.
>>
>> I want to know issues you see, so that I can correct them before putting
>> more
>> effort into this document.
>>
>>
>> Description:
>> Initial draft of PNaCl bitcode files document.
>>
>> BUG=None
>>
>> Please review this at https://codereview.chromium.org/266713003/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files (+4987, -0 lines):
>>   M chrome/common/extensions/docs/templates/json/chrome_sidenav.json
>>   A native_client_sdk/doc_generated/reference/pnacl-bitcode-manual.html
>>   M native_client_sdk/doc_generated/sitemap.html
>>   M native_client_sdk/src/doc/_book.yaml
>>   A native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst
>>   M native_client_sdk/src/doc/sitemap.rst
>>
>>
>> --
>> --
>> You received this message because you are subscribed to the Google
>> Groups "Native Client Engineering *INTERNAL*" group.
>> To post to this group, send email to nacl-eng@google.com
>> To unsubscribe from this group, send email to
>> nacl-eng+unsubscribe@google.com
>> For more options, visit this group at
>> http://groups.google.com/a/google.com/group/nacl-eng?hl=en
>>
>>
>  --
> --
> You received this message because you are subscribed to the Google
> Groups "Native Client Engineering *INTERNAL*" group.
> To post to this group, send email to nacl-eng@google.com
> To unsubscribe from this group, send email to
> nacl-eng+unsubscribe@google.com
> For more options, visit this group at
> http://groups.google.com/a/google.com/group/nacl-eng?hl=en
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

chromium-reviews

I patched in this CL and am serving it from my desktop here: http://ennay2.mtv.corp.google.com:8000/native-client/reference/pnacl-bitcode-manual On ...

6 years, 7 months ago (2014-05-02 17:26:18 UTC) #5

I patched in this CL and am serving it from my desktop here:
http://ennay2.mtv.corp.google.com:8000/native-client/reference/pnacl-bitcode-...


On Fri, May 2, 2014 at 10:18 AM, David Sehr <sehr@google.com> wrote:

> Raising it in your respective inboxes.
>
>
> On Thu, May 1, 2014 at 10:43 AM, <kschimpf@google.com> wrote:
>
>> Reviewers: nacl-eng_google.com,
>>
>> Message:
>> Here is my initial (incomplete) draft for the documentation for PNaCl
>> bitcode
>> files. I'm looking for feedback on style etc.
>>
>> I want to know issues you see, so that I can correct them before putting
>> more
>> effort into this document.
>>
>>
>> Description:
>> Initial draft of PNaCl bitcode files document.
>>
>> BUG=None
>>
>> Please review this at https://codereview.chromium.org/266713003/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files (+4987, -0 lines):
>>   M chrome/common/extensions/docs/templates/json/chrome_sidenav.json
>>   A native_client_sdk/doc_generated/reference/pnacl-bitcode-manual.html
>>   M native_client_sdk/doc_generated/sitemap.html
>>   M native_client_sdk/src/doc/_book.yaml
>>   A native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst
>>   M native_client_sdk/src/doc/sitemap.rst
>>
>>
>> --
>> --
>> You received this message because you are subscribed to the Google
>> Groups "Native Client Engineering *INTERNAL*" group.
>> To post to this group, send email to nacl-eng@google.com
>> To unsubscribe from this group, send email to
>> nacl-eng+unsubscribe@google.com
>> For more options, visit this group at
>> http://groups.google.com/a/google.com/group/nacl-eng?hl=en
>>
>>
>  --
> --
> You received this message because you are subscribed to the Google
> Groups "Native Client Engineering *INTERNAL*" group.
> To post to this group, send email to nacl-eng@google.com
> To unsubscribe from this group, send email to
> nacl-eng+unsubscribe@google.com
> For more options, visit this group at
> http://groups.google.com/a/google.com/group/nacl-eng?hl=en
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

binji

spellcheck pass https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right): https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst#newcode54 native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:54: The default abbreviations can be overriddeen with ...

6 years, 7 months ago (2014-05-02 18:04:52 UTC) #6

Jim Stichnoth

There are a few real comments apart from the spellcheck. https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right): https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst#newcode14 ...

6 years, 7 months ago (2014-05-08 12:55:16 UTC) #7

jvoung (off chromium)

also made a scan over this doc https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right): https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst#newcode20 native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:20: PNaClAsm focusses ...

6 years, 7 months ago (2014-05-27 23:18:50 UTC) #8

also made a scan over this doc

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right):

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:20: PNaClAsm
focusses on the semantic content of the file, not the
focuses

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:37: These block
records must be used like parentheses to define the block
how about "like balanced parentheses" ?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:91: Most of the
records, within a block, also define a unique values.
"define a unique values" -> "define unique values" ?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:113: Note: There is
one exception to this separation of local and global
How is this an exception?

For identifiers, locals only apply to the block it appears in (the current
function). Globals can be referenced by multiple block instances (multiple
functions).

I think what you want to clarify is that the global abbreviations, though
global, are tied to a specific type of block. E.g., there can be global function
abbreviations, that work on function blocks but don't work on value symbol table
blocks. The identifiers for those globals are therefore not unique (unless you
want to make the identifier of global abbreviations have a sort of namespacing).

E.g., instead of @a1 and @a1. Have @b4::a1 and @b3::a1.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:124: (explicitly)
passed to the PNaCl translator.
and downloaded through the internets ?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:142: use.
link to the special fwd ref record opcode?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:150: Types block
Is it worth splitting this into blocks that are expected to have one global
instance, and blocks that have multiple instances?

Or just within the description of each block clarify that.

Should the module block come first?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:152: program must
be defined in this block.
On 2014/05/08 12:55:17, stichnot wrote:
> Question.  Since PNaCl bitcode types are limited to i1, i8, i16, i32, i64,
f32,
> and f64, why bother listing them?
> 
> Edit: I see my question is answered below.  Might not hurt to add a clarifying
> hint here:  "These types consist of primitive types as well as high level
> constructs such as vectors and function signatures."

Might be good to link to a section about the allowed types?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:156: constants,
used by the program. It also defines how each global
I'm not a grammar expert, but you might not need that comma:

 "constants, used by" ?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:177: Defines
abbreviations that are used to compress PNaCl records. This
The "Abbreviations block" formerly known as the "Block Info Block". Is it worth
mentioning this alias? I agree that it's clearer to call this the Abbreviations
block instead of "block info block", since we aren't using the block info block
for anything else (dropped SETRECORDNAME, etc.).

Clarify that this is the block for defining *global* abbreviations?

The other (local) DEFINE_ABBREV don't have a separate
ENTER_SUBBLOCK/EXIT_SUBBLOCK.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:182: block, and a
module block.
Is it worth mentioning that in the next couple of sections, you will describe a
few more constraints on how the blocks are laid out?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:187: 
How does this look in the html? Should there be a bigger subsection dividing
line for each of the deep dives?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:206: module block
are function addresses.  All remaining definitions appear
function address / declarations ? Is it worth using the term declaration? At
least internal to LLVM it uses the term "declaration" as in "isDeclaration()".

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:308: special is
that they either apply in multiple blocks, or don't occur
How about these record codes as being "global", in contrast to other record
codes which are local to a specific type of block? Is that more clear than 
"either apply in multiple blocks, or don't occur in any block"?

Because they are global, they can't be the same number as the local record
codes, and therefore have gone to greater lengths to put them far away, close to
2**16?

In the actual bitstream, they have special reserved abbreviation IDs which are
small, and that makes it less of an issue in terms of # bits emitted. They are
also special in the sense that they use these special reserved abbreviations
(which are defined like user-defined abbreviations), and I'm not sure if this is
the best time to mention that bit of trivia. Mentioning it now does help
identify them, rather then leaving them unknown (but considered special).

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:330: default
abbreviation, no additional notation is used. Otherwise, the
That's not true, right? pnacl-objdump currently prints the 0-3: ?

I guess you are talking about if you were to make a parser for that
pnacl-objdump dump.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:367: The PNaClAsm
assembler can be viewed as a parser of PNaCl records. The
Were you still planning on writing the assembler, or is it just used for
illustrative purposes. A way to structure the documentation:

"How to write your own PNaCl bitcode reader, by example"?

This way of structuring things does help people understand the constraints, what
types of counts are written out in headers, how terminators work, etc. so that's
good.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:380: and the
corresponding syntax.
Clarify where other high-level semantics will be defined? (like divide by 0
should trap) -- link to other PNaCl ABI document?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:427: an instruction
that adds to two values (*O1* and *O2*) to generate
"adds to two values" -> "adds two values" ?

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:429: are all of
type *T*. Since abbreviation ID *A* is present,  the
extra space

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1281: Glboal
Constant Addresses
Global

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2397: *I* is the
(optional) abbreviation index associated with the record.
Is it necessary to put these "I: " under all the Record descriptions?

In the end, any record can optionally be abbreviated by some user-defined
abbreviation. So, isn't it a bit repetitive to mention it all the time (yet
sometimes leave the mention out, e.g., for conditional branch, even though
someone could make a user-defined abbrev for conditional branch)?  It's also
left out for the special 2**16 - 1, even though that is where the abbreviation
is special and is *not* optional in terms of writing to the bitstream.

It just happens to be that we have some common "user-defined" abbreviations that
*our* bitcode writer writes out all the time?

Karl

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right): https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst#newcode14 native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:14: This document is a reference manual for the contents ...

6 years, 6 months ago (2014-06-02 22:39:28 UTC) #9

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right):

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:14: This document
is a reference manual for the contents of PNaCl bitcode
On 2014/05/08 12:55:17, stichnot wrote:
> Formatting nit.  Looks like your emacs fill column is set to 70 or 72.  How
> about 80?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:16: uses a *static
single assignment* (SSA) model, based a representation
On 2014/05/08 12:55:17, stichnot wrote:
> This sentence doesn't parse.  Do you mean something like "... (SSA) based
> representation that ..." ?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:20: PNaClAsm
focusses on the semantic content of the file, not the
On 2014/05/27 23:18:50, jvoung wrote:
> focuses

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:22: that allows one
to specify how the PNaCl bitcode writer converts the
On 2014/05/08 12:55:17, stichnot wrote:
> allows --> allow

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:37: These block
records must be used like parentheses to define the block
On 2014/05/27 23:18:50, jvoung wrote:
> how about "like balanced parentheses" ?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:42: The *PNaCl
bitcode wrtier* takes the sequence of records, defined by a
On 2014/05/08 12:55:17, stichnot wrote:
> writer

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:43: PNaClAsm
program, and coverts each record into a (variable) sequence
On 2014/05/08 12:55:17, stichnot wrote:
> converts

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:48: For every kind
of record, there are default methods for converting
On 2014/05/08 12:55:17, stichnot wrote:
> "..., there is a default method ...", right?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:54: The default
abbreviations can be overriddeen with user-specified
On 2014/05/02 18:04:52, binji wrote:
> overridden 

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:55: abbrreviations.
 All user-specified abbreviations are included in the
On 2014/05/02 18:04:52, binji wrote:
> abbreviations

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:66: contents of
records into bit sequenes.  The main reason for defining
On 2014/05/02 18:04:52, binji wrote:
> sequences

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:73: notion of data
compression is cleanly seperated from semantic
On 2014/05/02 18:04:52, binji wrote:
> separated

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:77: For a JIT
translator, little (if any) compression should be
On 2014/05/08 12:55:17, stichnot wrote:
> For PNaCl, we've been consistently using "translator" to mean the compiler
that
> translates from PNaCl bitcode to native code.  Can you instead say "JIT
> compiler", and maybe also clarify that you mean a JIT compiler that produces
> PNaCl bitcode?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:81: delivering
accross the web, one may want to compress the sequence of
On 2014/05/02 18:04:52, binji wrote:
> across

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:88: can be nested
within other blocks. Each *block* defines a sequence of
On 2014/05/08 12:55:17, stichnot wrote:
> Weird inconsistency involving capitalization and italicization of "block".  I
> think you want this:
> 
> A program is defined as a sequence of top-level *blocks*. Blocks
> can be nested within other blocks. Each block defines a sequence of
> records.

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:91: Most of the
records, within a block, also define a unique values.
On 2014/05/27 23:18:50, jvoung wrote:
> "define a unique values" -> "define unique values" ?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:93: (i.e. *ID*). In
PNaClAms. each kind of block defines it own kind of
On 2014/05/02 18:04:52, binji wrote:
> PNaClAsm

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:93: (i.e. *ID*). In
PNaClAms. each kind of block defines it own kind of
On 2014/05/02 18:04:52, binji wrote:
> PNaClAsm

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:95: concatinating a
prefix character ('@' or '%'), the kind of block (a
On 2014/05/02 18:04:52, binji wrote:
> concatenating

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:105: multiple
functions, and therefore must be unique accross all function
On 2014/05/08 12:55:17, stichnot wrote:
> across

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:113: Note: There is
one exception to this separation of local and global
On 2014/05/27 23:18:50, jvoung wrote:
> How is this an exception?
> 
> For identifiers, locals only apply to the block it appears in (the current
> function). Globals can be referenced by multiple block instances (multiple
> functions).
> 
> I think what you want to clarify is that the global abbreviations, though
> global, are tied to a specific type of block. E.g., there can be global
function
> abbreviations, that work on function blocks but don't work on value symbol
table
> blocks. The identifiers for those globals are therefore not unique (unless you
> want to make the identifier of global abbreviations have a sort of
namespacing).
> 
> E.g., instead of @a1 and @a1. Have @b4::a1 and @b3::a1.

I did not like the idea of adding abbreviation namespacing, so I did not add it.
However, I agree that this paragraph needed rewriting, and the need to clarify
uniqueness of identifiers. I rewrote it (and moved it before the previous
paragraph).

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:116: in. If it is
global, the abberviation can apply to multiple block
On 2014/05/02 18:04:52, binji wrote:
> abbreviation

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:124: (explicitly)
passed to the PNaCl translator.
On 2014/05/27 23:18:50, jvoung wrote:
> and downloaded through the internets ?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:127: topologically
sorted, putting value definitions before thier uses.
On 2014/05/02 18:04:52, binji wrote:
> their

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:129: corresponding
information from it's uses.
On 2014/05/08 12:55:17, stichnot wrote:
> it's --> its
> 
> Also, there is an agreement problem, should probably be one of the following:
> 
> This implies that a record does not need to encode data if it can use the
> corresponding information from its uses.
> 
> This implies that records do not need to encode data if they can use the
> corresponding information from their uses.

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:136: However, For
function blocks (which define instructions), no
On 2014/05/08 12:55:17, stichnot wrote:
> For --> for

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:139: notion of a
forward (instruction value) declarations. These
On 2014/05/08 12:55:17, stichnot wrote:
> declarations --> declaration

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:140: decalrations
must appear before any of the uses of that value, if the
On 2014/05/02 18:04:52, binji wrote:
> declarations

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:141: (instruction)
value is defined later in the function, than its first
On 2014/05/08 12:55:17, stichnot wrote:
> I would drop the comma.

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:142: use.
On 2014/05/27 23:18:50, jvoung wrote:
> link to the special fwd ref record opcode?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:150: Types block
On 2014/05/27 23:18:50, jvoung wrote:
> Is it worth splitting this into blocks that are expected to have one global
> instance, and blocks that have multiple instances?
> 
> Or just within the description of each block clarify that.
> 
> Should the module block come first?

Moved the module block first. Added clarifying paragraph, following this list,
that clarifies which blocks define global identifiers, and can appear only once,
and those that can be repeated.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:152: program must
be defined in this block.
On 2014/05/08 12:55:17, stichnot wrote:
> Question.  Since PNaCl bitcode types are limited to i1, i8, i16, i32, i64,
f32,
> and f64, why bother listing them?
> 
> Edit: I see my question is answered below.  Might not hurt to add a clarifying
> hint here:  "These types consist of primitive types as well as high level
> constructs such as vectors and function signatures."

Good suggestion. done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:156: constants,
used by the program. It also defines how each global
On 2014/05/27 23:18:50, jvoung wrote:
> I'm not a grammar expert, but you might not need that comma:
> 
>  "constants, used by" ?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:163: Each function
(implemented) in a program has it's own block that
On 2014/05/08 12:55:17, stichnot wrote:
> its

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:166: Constants
Block
On 2014/05/08 12:55:17, stichnot wrote:
> Block --> block

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:169: within
corresponding function block.
On 2014/05/08 12:55:17, stichnot wrote:
> within the corresponding

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:171: Modue block
On 2014/05/02 18:04:52, binji wrote:
> Module

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:177: Defines
abbreviations that are used to compress PNaCl records. This
On 2014/05/27 23:18:50, jvoung wrote:
> The "Abbreviations block" formerly known as the "Block Info Block". Is it
worth
> mentioning this alias? I agree that it's clearer to call this the
Abbreviations
> block instead of "block info block", since we aren't using the block info
block
> for anything else (dropped SETRECORDNAME, etc.).
> 
> Clarify that this is the block for defining *global* abbreviations?
> 
> The other (local) DEFINE_ABBREV don't have a separate
> ENTER_SUBBLOCK/EXIT_SUBBLOCK.

From what I can tell, the "block info block" of llvm only contains abbreviations
as well. That is why I decided to change the name. I see
more confusion being added to this document to reference the internal names
used by the implementation. For readers (and implementors) of new language
translators, none of this matters.

I tried to clarify that the abbreviations block only defines *global*
abbreviations.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:182: block, and a
module block.
On 2014/05/27 23:18:50, jvoung wrote:
> Is it worth mentioning that in the next couple of sections, you will describe
a
> few more constraints on how the blocks are laid out?

Added an initial paragraph (after the block list) clarifying that we aren't
doing a deep dive here. Nor are we defining layout constraints. Rather, the
paragraph states that later subsections will do these deep dives.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:187: 
On 2014/05/27 23:18:50, jvoung wrote:
> How does this look in the html? Should there be a bigger subsection dividing
> line for each of the deep dives?

What I may agree on is that I am probably introducing too deep a concept when I
introduce the types of identifier names associated with each blocks. Will
refactor identifier names into a separate section, or move them to the
corresponding subsection that describes the contents of that block.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:198: PNaClAsm
constructs allows one to use explicit type names, rather than
On 2014/05/08 12:55:17, stichnot wrote:
> allow

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:206: module block
are function addresses.  All remaining definitions appear
On 2014/05/27 23:18:50, jvoung wrote:
> function address / declarations ? Is it worth using the term declaration? At
> least internal to LLVM it uses the term "declaration" as in "isDeclaration()".

Removed this, as part of the removal of the deep dive.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:216: addresses, but
the size of the corresponding memory associated with
On 2014/05/08 12:55:17, stichnot wrote:
> This block defines not only the addresses, but also the size ...

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:227: is defined by
a record in the Valuesymtab block. Currently, only
On 2014/05/08 12:55:17, stichnot wrote:
> Valuesymtab --> valuesymtab

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:235: function. Each
function block defines the control-flow graph of the
On 2014/05/08 12:55:17, stichnot wrote:
> Usually "control flow graph" just refers to the basic blocks at a high level
and
> the edges connecting the basic blocks, and not the individual instructions
> within the basic blocks.  What you're describing seems more like "intermediate
> representation".  Or maybe if you want to draw the line at instructions and
> higher, "instruction representation"?

Simplified paragraph, and introduced notion of intermediate representation.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:270: *terminating*
[ref] (i.e. branch) instructions are used to determine
On 2014/05/08 12:55:17, stichnot wrote:
> i.e. --> e.g.

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:274: enclosing
function block. The purpose of the constant block is to
On 2014/05/08 12:55:17, stichnot wrote:
> constant --> constants

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:277: corresponds to
the (relative) position of constant defined in the
On 2014/05/08 12:55:17, stichnot wrote:
> of --> of the

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:285: blocks of a
particular kind. These abbreviations ares denoted by the
On 2014/05/08 12:55:17, stichnot wrote:
> are

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:294: specific kind
of block, but are not necessarily unique accross
On 2014/05/02 18:04:52, binji wrote:
> across

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:296: descriminator
(i.e. tag) within a block, to identify what type of
On 2014/05/02 18:04:52, binji wrote:
> discriminator

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:302: (and actually
started as LLVM records[ref]).  For backwards
On 2014/05/08 12:55:17, stichnot wrote:
> space before [ref] for consistency?  (This is the first instance, but there
are
> several more in the document.)

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:303: compatability,
old numbers have not been reused, leaving gaps in the
On 2014/05/02 18:04:52, binji wrote:
> compatibility

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:308: special is
that they either apply in multiple blocks, or don't occur
On 2014/05/27 23:18:50, jvoung wrote:
> How about these record codes as being "global", in contrast to other record
> codes which are local to a specific type of block? Is that more clear than 
> "either apply in multiple blocks, or don't occur in any block"?
> 
> Because they are global, they can't be the same number as the local record
> codes, and therefore have gone to greater lengths to put them far away, close
to
> 2**16?
> 
> In the actual bitstream, they have special reserved abbreviation IDs which are
> small, and that makes it less of an issue in terms of # bits emitted. They are
> also special in the sense that they use these special reserved abbreviations
> (which are defined like user-defined abbreviations), and I'm not sure if this
is
> the best time to mention that bit of trivia. Mentioning it now does help
> identify them, rather then leaving them unknown (but considered special).

I like the concept of local/global record codes.  Rewrote (and simplified) above
paragraphs, based on this concept.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:310: future growth
in PNaClAsm, these special records have record codes
On 2014/05/08 12:55:17, stichnot wrote:
> hmm, are we expecting lots of future growth in our bitcode specification?

This gap is intentional to make them clearly distinguishable.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:311: close to value
2**16. Note: Well-formed PNaCl bitcode files do not
On 2014/05/08 12:55:17, stichnot wrote:
> to the value

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:322: construct it
represents. All records must hava record code. Hence,
On 2014/05/02 18:04:52, binji wrote:
> have a

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:327: records for
the call instruction, can have arbitrary length.
On 2014/05/08 12:55:17, stichnot wrote:
> Are there any variable-length records besides call, phi, and switch
> instructions?  (Do types and initializers count?)  If not, maybe just
enumerate
> them all here?

Enumerated the cases out.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:330: default
abbreviation, no additional notation is used. Otherwise, the
On 2014/05/27 23:18:50, jvoung wrote:
> That's not true, right? pnacl-objdump currently prints the 0-3: ?
> 
> I guess you are talking about if you were to make a parser for that
> pnacl-objdump dump.

Yes, I am talking about the text form. I did notice that I was somewhat
inconsistent, since the records column of pnacl-bcdis always defines an
abbreviation index. I have tried to fix this document to reflect this reality.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:331: record is
prefixed with the the abbreviation ID *I* to use (wrt the
On 2014/05/08 12:55:17, stichnot wrote:
> please spell out "with respect to"

Removed the clause, since it didn't really provide any information anyway.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:341: this are left
to section on abbreviations[ref]. However, at the PNaCL
On 2014/05/02 18:04:52, binji wrote:
> PNaCl

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:349: length *U&).
The number of bits *B* specified for an enter
On 2014/05/08 12:55:17, stichnot wrote:
> *U& --> *U*

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:358: Like much of
PNaClAsm, PNaClAsm requires that you specify sizes
On 2014/05/08 12:55:17, stichnot wrote:
> "Like much of PNaClAsm, PNaClAsm requires ..." sounds oddly tautological.  I'm
> not sure what you *really* mean.  Can you clarify/rewrite?

Rewrote this paragraph.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:361: within an
enter block record, you must specify how bits will be used
On 2014/05/08 12:55:17, stichnot wrote:
> This seems to be the only paragraph written in the second person -- two uses
of
> "you".  Fix this?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:362: to hold
abbreviation indexes.
On 2014/05/08 12:55:17, stichnot wrote:
> indices, since you use that everywhere else

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:367: The PNaClAsm
assembler can be viewed as a parser of PNaCl records. The
On 2014/05/08 12:55:17, stichnot wrote:
> Remove the last "The".

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:372: updates are
part of the sementics of the corresponding record
On 2014/05/02 18:04:52, binji wrote:
> semantics

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:380: and the
corresponding syntax.
On 2014/05/27 23:18:50, jvoung wrote:
> Clarify where other high-level semantics will be defined? (like divide by 0
> should trap) -- link to other PNaCl ABI document?

Added sentence to clarify this?

That is, additional higher-level semantics will be given (or linked) in the
semantics section.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:401: abbreviation
identifiers, and the number of bits required to repressent
On 2014/05/02 18:04:52, binji wrote:
> represent

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:427: an instruction
that adds to two values (*O1* and *O2*) to generate
On 2014/05/27 23:18:50, jvoung wrote:
> "adds to two values" -> "adds two values" ?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:429: are all of
type *T*. Since abbreviation ID *A* is present,  the
On 2014/05/27 23:18:50, jvoung wrote:
> extra space

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:452: following
subsections describes each element of the parse state.
On 2014/05/08 12:55:17, stichnot wrote:
> describe

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:477: Associated
with each function identifier is it's type signature. This
On 2014/05/08 12:55:17, stichnot wrote:
> its

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:478: is different
than the type of the function identifer, since function
On 2014/05/02 18:04:52, binji wrote:
> identifier

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:487: In addition,
if a function address is defining, there is a corresponding
On 2014/05/08 12:55:17, stichnot wrote:
> is defined?  is being defined?

Rewrote to clarify it has an implementing block.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:488: implementation
associated with the function address. To capture this association,
On 2014/05/08 12:55:17, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:524: The number of
constants defined in a fucntion.
On 2014/05/02 18:04:52, binji wrote:
> function

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:544: The expected
number of types defined in the Types Block.
On 2014/05/08 12:55:17, stichnot wrote:
> Types Block --> types block

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:563: There are four
special PNaCl records, each having their own record
On 2014/05/08 12:55:17, stichnot wrote:
> their --> its

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:574: can be nested,
it can appear inside other blocks, as well as at the
On 2014/05/08 12:55:17, stichnot wrote:
> it --> one

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:581: Addreviation
On 2014/05/08 12:55:17, stichnot wrote:
> Abbreviation

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:642: <655335,
BlockID, B>
On 2014/05/08 12:55:17, stichnot wrote:
> Are these values 655335 and 655334 correct?  They seem suspiciously similar to
> 2**16-1 and 2**16-2, yet they appear here >20 times.

Hmm. Don't know what happened here. Fixing.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:683: and the
modules block appears within the module block.
On 2014/05/08 12:55:17, stichnot wrote:
> modules block --> module block

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:683: and the
modules block appears within the module block.
On 2014/05/08 12:55:17, stichnot wrote:
> modules block --> module block

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:736: and the
modules block appears within the module block.
On 2014/05/08 12:55:17, stichnot wrote:
> modules block --> module block

Actually, I was using the names defined earlier. Hence "module" and "types".

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:736: and the
modules block appears within the module block.
On 2014/05/08 12:55:17, stichnot wrote:
> appears --> appear

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:776: primitive
type.  All other types (i.e. function and vector)are
On 2014/05/08 12:55:17, stichnot wrote:
> space after parens

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:780: to apear
before the composite types that use them.  There are no
On 2014/05/02 18:04:52, binji wrote:
> appear

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:784: The following
subsections introduces each valid PNaClAsm type, and the
On 2014/05/08 12:55:17, stichnot wrote:
> introduce

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:790: defines a
type. The following subsections define valid records within
On 2014/05/08 12:55:17, stichnot wrote:
> defines --> define

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:792: these defining
records implicitly define the type ID that will be used
On 2014/05/08 12:55:17, stichnot wrote:
> The position of each defining record implicitly defines the type...

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:813: 3. A function,
taking 32-bit integer and float arguments, and returns void.
On 2014/05/08 12:55:17, stichnot wrote:
> "and returning" or "that returns"

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:816: that type, is
based on the position of the type within the types
On 2014/05/08 12:55:17, stichnot wrote:
> and is based

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:819: appear
immediatedly after the assignment to ID @tN-1.
On 2014/05/08 12:55:17, stichnot wrote:
> immediately

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:824: The *count
record* defines how many types are defined in the
On 2014/05/08 12:55:17, stichnot wrote:
> For this and other types of count records, what happens if the bitcode file
> gives multiple (differing) count records?  Validation error?  First one wins? 
> Last one wins?

This is currently undefined. Fixing code to verify it is first record.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:842: to define more
(or less) types than value *N*, within the enclosing
On 2014/05/08 12:55:17, stichnot wrote:
> less --> fewer

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:863: are 32 bit
integer and floating type.
On 2014/05/08 12:55:17, stichnot wrote:
> are are --> are

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:863: are 32 bit
integer and floating type.
On 2014/05/08 12:55:17, stichnot wrote:
> types

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:865: The
corresponding PNaCl Records are:
On 2014/05/08 12:55:17, stichnot wrote:
> Records --> records

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:878: The *void*
type record defines the void type, which corrresponds to
On 2014/05/02 18:04:52, binji wrote:
> corresponds

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:895: The void type
record defines the type that has no values and has no
On 2014/05/08 12:55:17, stichnot wrote:
> values --> value ?

Rewrote sentence.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:929: signed
component of in integer is not specified by the type. Rather,
On 2014/05/08 12:55:17, stichnot wrote:
> in integer --> an integer

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1087: TBD.
On 2014/05/08 12:55:17, stichnot wrote:
> TODO for missing content, for easier searching, here and many other places.

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1107: sequence of
records that defines how each global addresss is
On 2014/05/02 18:04:52, binji wrote:
> address

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1118: its
initalizer records. All simple initializer records define a
On 2014/05/02 18:04:52, binji wrote:
> initializer

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1120: by
concatenating corresponding sequence of bytes for each of its
On 2014/05/08 12:55:17, stichnot wrote:
> concatenating a corresponding

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1150: The third
record defines the global constant address *@g0*, and it's
On 2014/05/08 12:55:17, stichnot wrote:
> its

ditto.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1151: corresponding
memory layout alignment. The forth record defines to
On 2014/05/08 12:55:17, stichnot wrote:
> fourth

ditto.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1152: initialize
the constant with 8 bytes, all with the value zero. This
On 2014/05/08 12:55:17, stichnot wrote:
> This --> The

Ditto.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1155: The fifth
record defines the global variable address *@g1*, and it's
On 2014/05/08 12:55:17, stichnot wrote:
> its

Ditto.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1160: eighth record
initializes bytes 5 and 6 to zero. The size of *@g2* is
On 2014/05/08 12:55:17, stichnot wrote:
> What is @g2?  This is its first mention.

Ditto.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1215: data.  The
global variable address record must be immediatedly
On 2014/05/02 18:04:52, binji wrote:
> immediately

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1233: A global
varaible address record defines a global address for a global
On 2014/05/02 18:04:52, binji wrote:
> variable

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1234: variable. 
*A* is the alignment to for the global variable.  *I* is
On 2014/05/08 12:55:17, stichnot wrote:
> to for --> for
> (or "of")

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1240: ??? Valid
values for A. Section defining notion of memory alignments ???
On 2014/05/08 12:55:17, stichnot wrote:
> TODO

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1281: Glboal
Constant Addresses
On 2014/05/27 23:18:50, jvoung wrote:
> Global

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1286: constant
address record must be immediatedly followed by initializer
On 2014/05/02 18:04:52, binji wrote:
> immediately

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1311: ??? Valid
values for A. Section defining notion of memory alignments ???
On 2014/05/08 12:55:17, stichnot wrote:
> TODO

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1372: A zerofill
initializer record intializes a sequence of bytes,
On 2014/05/02 18:04:52, binji wrote:
> initializes

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1416: the the
corresponding bytes the memory will be initialized with.
On 2014/05/02 18:04:52, binji wrote:
> s/the //

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1432: A data record
defines a sequence of bytes *B1* throught *BN*, that
On 2014/05/02 18:04:52, binji wrote:
> through

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1433: intialize *N*
bytes of memory. *I* is the (optional) abbreviation
On 2014/05/02 18:04:52, binji wrote:
> initialize

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1572: specified
global (non-funciton) address *A*, modified by the (unsigned
On 2014/05/02 18:04:52, binji wrote:
> function

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1573: integer)
offset *V*. *N* is the absolute indexassociated with *A*. The
On 2014/05/02 18:04:52, binji wrote:
> index associated

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1628: The compound
initializer record must immediately follow a global variable/constant
On 2014/05/08 12:55:17, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1694: The module
block, like all blocks, are enclosed in a pair of
On 2014/05/08 12:55:17, stichnot wrote:
> are --> is

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1696: consists The
following records (in order):
On 2014/05/08 12:55:17, stichnot wrote:
> The --> of the

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1701: bitcode (ABI)
verion. The PNaCl bitcode (ABI) version defines what
On 2014/05/02 18:04:52, binji wrote:
> version

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1703: bitcode file.
The version record defines the version of the PNaC
On 2014/05/02 18:04:52, binji wrote:
> PNaCl

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1735: that only
names for instrinsic functions must be provided. Any
On 2014/05/02 18:04:52, binji wrote:
> intrinsic

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1802: function
addresses also imply a corresponding
On 2014/05/08 12:55:17, stichnot wrote:
> implies

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1821: **Semnatics**
On 2014/05/08 12:55:17, stichnot wrote:
> Semantics

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1825: address is
defining only if *P==0*. Otherwise, it is only declared.
On 2014/05/08 12:55:17, stichnot wrote:
> defined

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1904: The
corresopnding records for these two function addresses are:
On 2014/05/02 18:04:52, binji wrote:
> corresponding

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1925: A function
definition contains a list of basic block, forming the CFG
On 2014/05/08 12:55:17, stichnot wrote:
> blocks

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1927: instructions,
and ends with a *terminator* [ref] (branch) instruction.
On 2014/05/08 12:55:17, stichnot wrote:
> e.g., branch
> 
> since there are several kinds of terminator instructions

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1933: has no
predecessors, it also can't have any *PHI nodes* [ref].
On 2014/05/08 12:55:17, stichnot wrote:
> nodes --> instructions

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1939: The number of
basic blocks are defined by the count record. Each
On 2014/05/08 12:55:17, stichnot wrote:
> are --> is

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1940: termimintor
instruction ends the current basic block, and the next
On 2014/05/02 18:04:52, binji wrote:
> terminator

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1957: value, rather
than abolute. The is done because most instruction
On 2014/05/02 18:04:52, binji wrote:
> absolute

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1961: number. Small
numbers tend to require less bits when they are
On 2014/05/08 12:55:17, stichnot wrote:
> less --> fewer

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1993: The value of
*N* corresponds the the positional index of the
On 2014/05/08 12:55:17, stichnot wrote:
> On 2014/05/02 18:04:52, binji wrote:
> > s/the //
> 
> Actually, the the --> to the

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1995: with. *M* is
the number of defined paramaters (plus one)
On 2014/05/02 18:04:52, binji wrote:
> parameters

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2096: instuction
indicates which block should be executed after the current
On 2014/05/02 18:04:52, binji wrote:
> instruction

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2184: The return
instruction returns control to the calling function,
On 2014/05/08 12:55:17, stichnot wrote:
> return instruction --> return value instruction ?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2219: %v10 = add
i32 %v1, @v2;
On 2014/05/08 12:55:17, stichnot wrote:
> If you want, you could avoid the implicit forward reference to the Add
> instruction with an example that just returns a function argument.

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2287: defines the
following PNaCL record:
On 2014/05/02 18:04:52, binji wrote:
> PNaCl

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2356: %v10 = cmp eq
i32 %v8, %v9;
On 2014/05/08 12:55:17, stichnot wrote:
> Could avoid a forward reference to the Cmp instruction by making %v10 a
function
> argument, or perhaps introduce Cmp before Br.

Can't do this. Only i32 and i64 arguments are allowed. However, did complete
example to a complete function.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2374: instruction. 
The most common use of this is when one calls a
On 2014/05/08 12:55:17, stichnot wrote:
> A more tangible/recognizable example may be the low-level implementation of
> assert() or abort().

Decided to drop the clarification example.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2397: *I* is the
(optional) abbreviation index associated with the record.
On 2014/05/27 23:18:50, jvoung wrote:
> Is it necessary to put these "I: " under all the Record descriptions?
> 
> In the end, any record can optionally be abbreviated by some user-defined
> abbreviation. So, isn't it a bit repetitive to mention it all the time (yet
> sometimes leave the mention out, e.g., for conditional branch, even though
> someone could make a user-defined abbrev for conditional branch)?  It's also
> left out for the special 2**16 - 1, even though that is where the abbreviation
> is special and is *not* optional in terms of writing to the bitstream.
> 
> It just happens to be that we have some common "user-defined" abbreviations
that
> *our* bitcode writer writes out all the time?

I agree that it is a bit repetitive. However, I haven't found a way to remove
them yet that is clean. I will continue to look for an answer.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2442: the
corresponding signed integer operation. One can tell whether the
On 2014/05/08 12:55:17, stichnot wrote:
> I don't really like this attempt at generalization.  From my perspective,
there
> are 3 categories of binary instructions:
> 
> * Generic (non-signed) integer plus (signed) floating: add/fadd, sub/fsub,
> mul/fmul.  This category does fit nicely into the generalization.
> 
> * Generic (non-signed) integer-only: shl, lshr, ashr, and, or, xor.  Here,
> you'll have to make it explicit that FP types are not allowed.
> 
> * Signed+unsigned integer plus (signed) floating: sdiv/udiv/fdiv,
> srem/urem/frem.  For each of {div,rem}, there are actually 3 instructions.  In
> your generalization, you have to list two of them, and then point out that one
> but not the other disallows the FP type.  And if you list e.g. "div" and
"sdiv",
> I think the implication would be that e.g. "div" allows both unsigned integer
> and (signed) FP types, while "sdiv" allows signed integer types and no FP
types,
> which is a bit backwards - nicer would be "div" that allows signed integer and
> (signed) FP types, and "udiv" that allowed just unsigned integer types.
> 
> (I'm wondering if you chose this generalization because of the specific
encoding
> that isn't yet documented here?)
> 
> Given all this, I think it would be clearer to just enumerate the 18 binary
> instructions, each one's argument types being either signed int, unsigned int,
> non-signed int, or FP, rather than enumerating 11 binary instructions with
> implicit qualifications.

I refactored this into two sections: Integral binary operations and Floating
binary operations. Hopefully it now looks better.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2450: arguments,
and the result, must be an integer, floating, or vector
On 2014/05/08 12:55:17, stichnot wrote:
> "an" --> "the same", right?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2469: instruction. 
*I* is the (optiona) abbreviation associated with the
On 2014/05/02 18:04:52, binji wrote:
> optional

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2519: sdiv
On 2014/05/08 12:55:17, stichnot wrote:
> This should be rem and srem.

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2523: Shift left
Instruction
On 2014/05/08 12:55:17, stichnot wrote:
> left --> Left

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2530: Logical Shift
right Instructions
On 2014/05/08 12:55:17, stichnot wrote:
> right --> Right
> Instructions --> Instruction

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2533: ashr
On 2014/05/08 12:55:17, stichnot wrote:
> ashr --> lshr

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2537: Arithmetic
Shift right Instructions
On 2014/05/08 12:55:17, stichnot wrote:
> right --> Right

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2564: Stack frame
memory allocation Instruction
On 2014/05/08 12:55:17, stichnot wrote:
> Capitalize all words for consistency?

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2622: Phi
Instruction
On 2014/05/08 12:55:17, stichnot wrote:
> Be sure to mention that within any given basic block, all the Phi instructions
> must come before all the non-Phi instructions.

Agreed.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2627: Forward type
declarations
On 2014/05/08 12:55:17, stichnot wrote:
> I know what all the other instructions are, but I don't recognize this.  I
think
> you should move this to the beginning or end of the list of instruction kinds
so
> it doesn't break the flow of descriptions of instructions that essentially
> actually map to machine instructions.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2667: bitcode IDs
using a single *absolute* index. The abolute index for
On 2014/05/02 18:04:52, binji wrote:
> absolute

Done.

https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/re...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2712: * Block
Structue
On 2014/05/02 18:04:52, binji wrote:
> Structure

Done.

Jim Stichnoth

New round of spellcheck. https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right): https://codereview.chromium.org/266713003/diff/1/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst#newcode361 native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:361: within an enter block record, ...

6 years, 6 months ago (2014-06-06 18:24:50 UTC) #10

jvoung (off chromium)

couple more comments too https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right): https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst#newcode15 native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:15: is presented using assembly language ...

6 years, 6 months ago (2014-06-06 20:09:15 UTC) #11

couple more comments too

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right):

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:15: is presented
using assembly language *PNaClAsm*. PNaClAsm uses a *static single
Perhaps mention that there are three layers up front (PNaClAsm, PNaCl records,
bit sequencing with abbreviations), since this section introduces the three?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:24: Below PNaClAsm
is the high-level form of the data stored in PNaCl bitcode
"Below PNaClAsm is the high-level..."

Sounds a bit odd, like the description must have started at a pretty high level
for "below" to still have another "high-level" under it =)

Do you mean "abstraction" ?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:87: support is
fixed at IEEE 754 32-bit and 64-bit values (f32 and f64,
I think it's called "double" and "float" in the asm syntax actually?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:92: ===========
Feels a bit early to talk about identifiers. Should the Blocks be introduced
first? After all, the first sentence of this Identifiers section introduces
blocks. Can the blocks section be described without introducing identifiers?

Also, the identifiers depend on the specific blocks, and which specific blocks
are available aren't introduced yet.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:225: Each block,
within a bitcode file, defines values. These values are associated
regarding the earlier comment about swapping the order of Blocks and
Identifiers, maybe this specific paragraph can be moved to the beginning of the
identifiers block?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:229: and can have
mlutiple instances (one for each implemented function).
multiple

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:287: length.
Should this reference a section that describes how a reader might determine the
length then (or describe inline)?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:313: Abbreviation
index for the abbreviation used to bit-encode a user-defined
I don't know if it's too redundant, or clear as is, but my on first read the
phrase "a user-defined abbreviation record" didn't seem to denote strongly
enough that this where the user-supplied abbrevs get defined. Not sure if others
feel the same.

I think because of the semi-circularity: records are bit-encoded w/
abbreviations, but user-supplied abbreviation definitions are encoded as records
too!

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:335: Conventions
for describing records
nit: consistency in capitalization of section headers? (here plus elsewhere)

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:340: using syntax
rules, and semantics on how they are converted to records. The
This is the first time "the parser" is mentioned.

"The parser also has state" or "A PNaClAsm parser would also have state"?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:360: Within a
syntax rule, there may specifications about abbreviations.  These
"there may be specifications" ?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:381: literal while
the upper case sequence of letters denote (rule specific)
Earlier "all alphabetic characters are lower case unless they appear in a
literal value" (makes me think literal values will be upper case).

Now "the lower case letters are literal while the upper case... denote values"
(makes me think literal values will be lower case)

?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:441: 40:0|  1:
<65535, 17, 4>          |  types {  // BlockID = 17
Abbrev index bitwidth says "4", even though no user-defined abbrevs are defined.
I guess this is using the laxness of the rule that says the bitwidth doesn't
need to be minimal + a quirk in the way your tool erases abbreviations after the
fact.

Maybe for the example, just pretend abbrevs were erased optimally and say 2? 
Depends on if/what you add below, describing some of column 2.

Up to you... my reasoning is that reproducing this result where there are 0
abbreviations takes several steps. Also, if someone was to make a from-scratch
writer w/ no user-defined abbreviations, they would probably do it so that this
entry is "2".

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:497: Byte 13
defines the PNaCl bitcode version of the file. Currently, only version 2
Only byte 13, or byte 13, 14, 15, and 16?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:498: is allowed.
What about the bytes in between (5 through 12)? Considered opaque, or must be X,
Y, Z?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:520: fact(n-1)".
The second column isn't really described in this example?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:552: different than
the type of the function identifier, since function identifiers
would it help to say that "..., since function identifiers represent the
function  address which is a pointer (and pointers are always implemented as a
32-bit integer following the ILP32 data model)." or something to that effect?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:582:
NumDefinedFcnAddresses
How is this different from NumFuncAddresses? Update rules only mention
NumFuncAddresses.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:597: The number of
constants defined in a function.
so far?

Depends on how you define the update rule in constants blocks.

Seems like NumParams is the only one where you know up front how many there
should be (via the function signature).

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:815: 
Description for the remaining global record (abbreviation definition records)?
Or refer to a later section?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1060: PNaClAsm
allows computation on 32-bit floating values. A floating type record
"floating type record" --> "float type record" ?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2590: section
focusses on binary instructions that operator on integral values, or
focuses

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2631: Overflow
conditions are ignored, and the result returned is the mathematical
"Overflow conditions are ignored, and ..."

Is "ignored" meant to refer to the LLVM nsw, nuw, overflow behavior attributes?
If so, I think you can omit the "ignored" part since nsw/nuw aren't even
mentioned anywhere.  Just go ahead and say overflow is supposed to be handled
via the modulo?

Similar for other instances?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2876: undefined.
Maybe keep the clarification that overflow happens when dividing the max
negative negative integer by -1?

I wonder if this is a bit of undefined behavior we could just make defined, but
at the cost of some performance -- similar to divide by zero. E.g., Java makes
the result stay as the max negative number.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2954: disallowed.
Division by zero is guaranteed to trap. Overflow is also undefined.
When would overflow happen, if these are unsigned? (for the signed case this
happens when you divide by -1)

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3110: Division by
zero is guaranteed to trap. Overflow is also undefined.
similar, overflow?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3632: Floating
Binary Instructions
Floating Point Binary?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4104: of value to
read. *V* is the alignment of the memory address.  *A* is the
Should we say more about alignment? What happens if the alignments don't match
what is in the table below?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4266: record. Both
*T1* and *T2* must be integer types, or integral vectors of the
same number of elements instead of same "size" ?

T1 has to be wider than T2 (no nops or expansions)

If the value doesn't fit in T2, then the higher order bits dropped (vs fptrunc
which says it's undefined behavior if value cannot be represented precisely as a
less precise float)

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4274: TypeOf(V) =
T1
Earlier you use == for these TypeOf constraints, but here and below you use =.
This is not meant to be an update like in the Updates section?

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4333: record. Both
*T1* and *T2* must be integer types, or integral vectors of the
floating point types / vectors (seems copy pasted from trunc)

T1 has to be wider than T2 (so that it's a truncation).

Karl

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right): https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst#newcode15 native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:15: is presented using assembly language *PNaClAsm*. PNaClAsm uses a ...

6 years, 5 months ago (2014-06-30 22:09:06 UTC) #12

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
File native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst (right):

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:15: is presented
using assembly language *PNaClAsm*. PNaClAsm uses a *static single
On 2014/06/06 20:09:14, jvoung wrote:
> Perhaps mention that there are three layers up front (PNaClAsm, PNaCl records,
> bit sequencing with abbreviations), since this section introduces the three?

Good suggestion. Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:24: Below PNaClAsm
is the high-level form of the data stored in PNaCl bitcode
On 2014/06/06 20:09:15, jvoung wrote:
> "Below PNaClAsm is the high-level..."
> 
> Sounds a bit odd, like the description must have started at a pretty high
level
> for "below" to still have another "high-level" under it =)
> 
> Do you mean "abstraction" ?

Decided to just call it the "textual" form.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:87: support is
fixed at IEEE 754 32-bit and 64-bit values (f32 and f64,
On 2014/06/06 20:09:13, jvoung wrote:
> I think it's called "double" and "float" in the asm syntax actually?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:92: ===========
On 2014/06/06 20:09:14, jvoung wrote:
> Feels a bit early to talk about identifiers. Should the Blocks be introduced
> first? After all, the first sentence of this Identifiers section introduces
> blocks. Can the blocks section be described without introducing identifiers?
> 
> Also, the identifiers depend on the specific blocks, and which specific blocks
> are available aren't introduced yet.

Good point. Moving after PNaCl blocks.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:216: This section
is only intented as a high-level discussion of blocks. Later
On 2014/06/06 18:24:49, stichnot wrote:
> intended

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:259: (staring from
zero). In an ideal world, they would be a consecutive sequence of
On 2014/06/06 18:24:49, stichnot wrote:
> starting

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:287: length.
On 2014/06/06 20:09:14, jvoung wrote:
> Should this reference a section that describes how a reader might determine
the
> length then (or describe inline)?

Added a couple of sentences trying to clarify this.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:289: Records are
converted to bit sequences using an abbreviation. Let *a* the the index
On 2014/06/06 18:24:50, stichnot wrote:
> the the --> be the
> also, 80-col

Removed paragraph. presented too early to be useful.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:296: contents of
the record. The details of this are left to section on abbreviations
On 2014/06/06 18:24:47, stichnot wrote:
> to section --> to the section
> (though this will probably go away when refs are filled in...)

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:309: Abbreviation
index for the abbreviation used to bit-encode a enter block
On 2014/06/06 18:24:47, stichnot wrote:
> a enter --> an enter

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:313: Abbreviation
index for the abbreviation used to bit-encode a user-defined
On 2014/06/06 20:09:14, jvoung wrote:
> I don't know if it's too redundant, or clear as is, but my on first read the
> phrase "a user-defined abbreviation record" didn't seem to denote strongly
> enough that this where the user-supplied abbrevs get defined. Not sure if
others
> feel the same.
> 
> I think because of the semi-circularity: records are bit-encoded w/
> abbreviations, but user-supplied abbreviation definitions are encoded as
records
> too!

I agree this isn't obvious. Added a sentence to help explain.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:340: using syntax
rules, and semantics on how they are converted to records. The
On 2014/06/06 20:09:14, jvoung wrote:
> This is the first time "the parser" is mentioned.
> 
> "The parser also has state" or "A PNaClAsm parser would also have state"?

Tried to clear up that there is "global" state that gets updated by syntax
rules, and is used to capture positional context.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:381: literal while
the upper case sequence of letters denote (rule specific)
On 2014/06/06 20:09:13, jvoung wrote:
> Earlier "all alphabetic characters are lower case unless they appear in a
> literal value" (makes me think literal values will be upper case).
> 
> Now "the lower case letters are literal while the upper case... denote values"
> (makes me think literal values will be lower case)
> 
> ?

Rewrote. Hopefully clarified.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:416: This section
provides a simple example of a PNaCl bticode file. Its contents
On 2014/06/06 18:24:49, stichnot wrote:
> bitcode

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:429: Compiling this
into a PEXE file, and dumping out its contents with utility
On 2014/06/06 18:24:48, stichnot wrote:
> Just checking - are we calling this a PEXE file in prose, or something like
> PNaCl bitcode file?

Good point. I should call it the bitcode file, not a PEXE file to be consistent.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:441: 40:0|  1:
<65535, 17, 4>          |  types {  // BlockID = 17
On 2014/06/06 20:09:13, jvoung wrote:
> Abbrev index bitwidth says "4", even though no user-defined abbrevs are
defined.
> I guess this is using the laxness of the rule that says the bitwidth doesn't
> need to be minimal + a quirk in the way your tool erases abbreviations after
the
> fact.
> 
> Maybe for the example, just pretend abbrevs were erased optimally and say 2? 
> Depends on if/what you add below, describing some of column 2.
> 
> Up to you... my reasoning is that reproducing this result where there are 0
> abbreviations takes several steps. Also, if someone was to make a from-scratch
> writer w/ no user-defined abbreviations, they would probably do it so that
this
> entry is "2".

Thanks for noting this. I didn't realize that we should fix this value when
removing abbreviations in pnacl-bccompress.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:496: the file. That
is, 'PEXE'. All PEXE bitcode files begin with these four bytes.
On 2014/06/06 18:24:50, stichnot wrote:
> the file, i.e. 'PEXE'.

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:497: Byte 13
defines the PNaCl bitcode version of the file. Currently, only version 2
On 2014/06/06 20:09:13, jvoung wrote:
> Only byte 13, or byte 13, 14, 15, and 16?

Simplified this to just state they shouldn't change for PNaCl version 2 files.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:498: is allowed.
On 2014/06/06 20:09:14, jvoung wrote:
> What about the bytes in between (5 through 12)? Considered opaque, or must be
X,
> Y, Z?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:520: fact(n-1)".
On 2014/06/06 20:09:13, jvoung wrote:
> The second column isn't really described in this example?

yes.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:552: different than
the type of the function identifier, since function identifiers
On 2014/06/06 20:09:14, jvoung wrote:
> would it help to say that "..., since function identifiers represent the
> function  address which is a pointer (and pointers are always implemented as a
> 32-bit integer following the ILP32 data model)." or something to that effect?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:582:
NumDefinedFcnAddresses
On 2014/06/06 20:09:14, jvoung wrote:
> How is this different from NumFuncAddresses? Update rules only mention
> NumFuncAddresses.

These are two different values. Added additional sentences to clarify.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:597: The number of
constants defined in a function.
On 2014/06/06 20:09:13, jvoung wrote:
> so far?
> 
> Depends on how you define the update rule in constants blocks.
> 
> Seems like NumParams is the only one where you know up front how many there
> should be (via the function signature).

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:815: 
On 2014/06/06 20:09:14, jvoung wrote:
> Description for the remaining global record (abbreviation definition records)?
> Or refer to a later section?

This was already stated in the enclosing section.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1186: arguments.
Indicies to the corresponding type identifiers is stored in the
On 2014/06/06 18:24:48, stichnot wrote:
> are stored

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1186: arguments.
Indicies to the corresponding type identifiers is stored in the
On 2014/06/06 18:24:48, stichnot wrote:
> Indices

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1446: *V* caluse
can be omitted if *V* is zero. *A* is the (optional) abbreviation
On 2014/06/06 18:24:48, stichnot wrote:
> clause

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:1857: file. The
version record defines the version of the PNaCL bitcode
On 2014/06/06 18:24:48, stichnot wrote:
> PNaCL --> PNaCl

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2089: instruction
that follows a temrinator instruction begins a new basic block.
On 2014/06/06 18:24:48, stichnot wrote:
> terminator

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2119: indices for
each kind of identifier.  That is, Indices are ordered by putting
On 2014/06/06 18:24:47, stichnot wrote:
> Indices --> indices

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2125: than
absolute. The is done because most instruction operands refer to values
On 2014/06/06 18:24:49, stichnot wrote:
> The is done --> This is done

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2625: The integer
add instruction returns the sum of its two arguments. Arguments *V1* and
On 2014/06/06 18:24:47, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2631: Overflow
conditions are ignored, and the result returned is the mathematical
On 2014/06/06 20:09:15, jvoung wrote:
> "Overflow conditions are ignored, and ..."
> 
> Is "ignored" meant to refer to the LLVM nsw, nuw, overflow behavior
attributes?
> If so, I think you can omit the "ignored" part since nsw/nuw aren't even
> mentioned anywhere.  Just go ahead and say overflow is supposed to be handled
> via the modulo?
> 
> Similar for other instances?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2637: In the add
instruction, Integral type i1 (and vectors on integral type i1) is
On 2014/06/06 18:24:49, stichnot wrote:
> Integral --> integral

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2785: The intebger
multiply instruction returns the product of its two
On 2014/06/06 18:24:46, stichnot wrote:
> integer

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2798: In the
subtract instruction, Integral type i1 is disallowed.
On 2014/06/06 18:24:47, stichnot wrote:
> Integral --> integral

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2876: undefined.
On 2014/06/06 20:09:14, jvoung wrote:
> Maybe keep the clarification that overflow happens when dividing the max
> negative negative integer by -1?
> 
> I wonder if this is a bit of undefined behavior we could just make defined,
but
> at the cost of some performance -- similar to divide by zero. E.g., Java makes
> the result stay as the max negative number.

While I agree we should consider tightening this, I don't know the cost.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2953: In the
unsigned integer divide instruction, Integral type i1 is
On 2014/06/06 18:24:46, stichnot wrote:
> Integral --> integral

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:2954: disallowed.
Division by zero is guaranteed to trap. Overflow is also undefined.
On 2014/06/06 20:09:14, jvoung wrote:
> When would overflow happen, if these are unsigned? (for the signed case this
> happens when you divide by -1)

Removing sentence.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3110: Division by
zero is guaranteed to trap. Overflow is also undefined.
On 2014/06/06 20:09:14, jvoung wrote:
> similar, overflow?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3632: Floating
Binary Instructions
On 2014/06/06 20:09:15, jvoung wrote:
> Floating Point Binary?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3635: Floating
Binary instructions require two operands of the same type, execute an
On 2014/06/06 18:24:49, stichnot wrote:
> Do you want binary capitalized?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3710: The
floatsubtract instruction returns the difference of its two arguments. Both
On 2014/06/06 18:24:46, stichnot wrote:
> float subtract

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3712: or an
floating based vector type.
On 2014/06/06 18:24:48, stichnot wrote:
> an floating --> a floating

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3730: *T*. *T* must
be an floating type, or a floating vector type. *N* is defined by
On 2014/06/06 18:24:49, stichnot wrote:
> a floating

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:3732: instruction.
*A* is the (optional) abbreviation ¯associated with the
On 2014/06/06 18:24:49, stichnot wrote:
> You have an interesting unicode character between "abbreviation" and
> "associated".

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4018: stack. *S*
must be of integral type i32. *V* is the aligment of the generated
On 2014/06/06 18:24:47, stichnot wrote:
> alignment

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4102: The load
instruction is used to read from memory. *P* is identifier of the
On 2014/06/06 18:24:48, stichnot wrote:
> is the identifier

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4104: of value to
read. *V* is the alignment of the memory address.  *A* is the
On 2014/06/06 20:09:14, jvoung wrote:
> Should we say more about alignment? What happens if the alignments don't match
> what is in the table below?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4108: are allowed
for floating types. All integral types except i1 is allowed.
On 2014/06/06 18:24:47, stichnot wrote:
> are allowed

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4188: are allowed
for floating types. All integral types except i1 is allowed.
On 2014/06/06 18:24:46, stichnot wrote:
> are allowed

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4242: Integer
truncating Instruction
On 2014/06/06 18:24:48, stichnot wrote:
> capitalization?

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4266: record. Both
*T1* and *T2* must be integer types, or integral vectors of the
On 2014/06/06 20:09:14, jvoung wrote:
> same number of elements instead of same "size" ?
> 
> T1 has to be wider than T2 (no nops or expansions)
> 
> If the value doesn't fit in T2, then the higher order bits dropped (vs fptrunc
> which says it's undefined behavior if value cannot be represented precisely as
a
> less precise float)

This was implied by the constratiants, but I agree it is worth mentioning
verbally also.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4274: TypeOf(V) =
T1
On 2014/06/06 20:09:15, jvoung wrote:
> Earlier you use == for these TypeOf constraints, but here and below you use =.
> This is not meant to be an update like in the Updates section?

You are right. I am inconsistent with this. Fixing...

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4333: record. Both
*T1* and *T2* must be integer types, or integral vectors of the
On 2014/06/06 20:09:14, jvoung wrote:
> floating point types / vectors (seems copy pasted from trunc)
> 
> T1 has to be wider than T2 (so that it's a truncation).

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4385: bit size of
the value must be smaller than the bitsize of the destination
On 2014/06/06 18:24:47, stichnot wrote:
> bitsize --> bit size

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4455: The sign
extending instruction takes an value to cast, and a type to
On 2014/06/06 18:24:47, stichnot wrote:
> a value

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4458: bitsize of
the destination type. Equal sized types are not allowed.
On 2014/06/06 18:24:47, stichnot wrote:
> bit size

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4593: The
SignRotate function encodes a signed integer in an easily compressable
On 2014/06/06 18:24:48, stichnot wrote:
> compressible

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4649: internal
abbreviation index saved in the bitcode file. It adds 4 to its argument,
On 2014/06/06 18:24:47, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4684: vector is the
underlying type.
On 2014/06/06 18:24:47, stichnot wrote:
> What if the type is a vector of vectors?  (or is that even possible?)

This isn't possible.

https://codereview.chromium.org/266713003/diff/20001/native_client_sdk/src/do...
native_client_sdk/src/doc/reference/pnacl-bitcode-manual.rst:4729: The
*abbreviations block* [ref] is the first block in the module buld.. The
On 2014/06/06 18:24:46, stichnot wrote:
> buld?

Done.

Karl

6 years, 5 months ago (2014-06-30 22:09:39 UTC) #13

Closing issue. Client to old to properly update.

See CL https://codereview.chromium.org/364463002

Issue 266713003: Initial draft of PNaCl bitcode files document. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Second version of document. #

Messages