The perfect is the enemy of the good.  

>From all I have seen and heard, this rewrite is a clear improvement 
over the status quo.  So I am going to review and approve it wearing
my doc maintainer hat, deferring to and relying on Andrew and Richard 
and their deep expertise on the technical side.

Please take my comments below into account for an updated patch, and 
once Andrew and Richard have signed of, this is then good to commit.

(Patches of this size really are tough to review.  That surely has
contributed to the delay getting this in the tree most significantly.
In fact, I fell asleep over my notebook thrice reviewing this last
evening, though traveling from US West coast to Central Europe without
sleep during the flight probably did not exactly help, either. :-)

On Fri, 25 Apr 2014, James Greenhalgh wrote:
>> +@menu
>> +* Basic Asm::          Inline assembler with no operands.
>> +* Extended Asm::       Inline assembler with operands.
>> +* Constraints::        Constraints for asm operands

Should this be "Asm operands" based on the other references here
or, better @code{asm} in all cases here?

>> +* Explicit Reg Vars::  Defining variables residing in specified registers.

Should this be "Register" instead of just "Reg"?
And "Variables" instead of just "Vars"?

>> +* Size of an asm::     How GCC calculates the size of an asm block.

@code{asm} ?

>> +@subsection Basic Asm - Assembler Instructions with No Operands

--- instead of -

>> +To create headers compatible with ISO C, write @code{__asm__} instead of 
>> +@code{asm} (@pxref{Alternate Keywords}).

Here it's just ISO C, whereas in case of __inline__ you had ISO C90.
Would it make sense to just use ISO C throughout?

>> +By definition, a Basic @code{asm} statement is one with no operands. 
>> +@code{asm} statements that contain one or more colons (used to delineate 
>> +operands) are considered to be Extended (for example, @code{asm("int $3")} 
>> +is Basic, and @code{asm("int $3" : )} is Extended). @xref{Extended Asm}.

At this point the reader does not yet know the concept of those
colons, does she?  So that can be seen as a bit confusing.

>> +@subsubheading Parameters
>> +@emph{AssemblerInstructions}
>> +@*
>> +This is a literal string that specifies the assembler code. The string can 

Just "assembler code", without "the", would be my take, but let's see
what native speakers so.

>> +The GCC compiler does not parse the assembler instructions themselves and 

"GCC does".  GCC stands for GNU Compiler Collection, and GNU Compiler
Collection Compiler may be a bit confusing. ;-)

>> +You may place multiple assembler instructions together in a single 
>> @code{asm} 
>> +string, separated by the characters normally used in assembly code for the 
>> +system. 

Might "by the same characters normally...the target system" be more clear?

>> +A combination that works in most places is a newline to break the 
>> +line, plus a tab character to move to the instruction field (written 
>> +as "\n\t").

Will everyone know what an instruction field is?  I'm not sure it's
that common of a term.

>> +Do not expect a sequence of @code{asm} statements to remain perfectly 
>> +consecutive after compilation. If certain instructions need to remain 
>> +consecutive in the output, put them in a single multi-instruction asm 
>> +statement. Note that GCC's optimizer can move @code{asm} statements 
>> +relative to other code, including across jumps.

optimizers (plural)

>> +GCC's optimizer does not know about these jumps, and therefore cannot take 
>> +account of them when deciding how to optimize. Jumps from @code{asm} to C 
>> +labels are only supported in Extended @code{asm}.

How about just saying "GCC does not know..:"?

>> +@subsubheading Remarks
>> +Using Extended @code{asm} will typically produce smaller, safer, and more 
>> +efficient code, and in most cases it is a better solution.

Why?

>> When writing inline assembly language outside C functions, however, 
>> you must use Basic @code{asm}. Extended @code{asm} statements have to 
>> be inside a C function.

Might that be "outside of C functions"?  Native speakers?

The way this is phrased sounds a bit negative.  Is this really a
drawback?

>> +Under certain circumstances, GCC may duplicate (or remove duplicates of) 
>> your 
>> +asm code as part of optimization. This can lead to unexpected duplicate 
>> +symbol errors during compilation if your asm code defines symbols or labels.

"as part of optimization" -> "when optimizing"

@code{asm}

>> +Safely accessing C data and calling functions from Basic @code{asm} is more 
>> +complex than it may appear. To access C data, it is better to use Extended 
>> +@code{asm}.

Here we are, this looks like (part of) the answer to my question
"Why?" above. :-)

>> +Since GCC does not parse the AssemblerInstructions, it has no visibility of 
                                 ^^^^^^^^^^^^^^^^^^^^^
Something wrong here.

>> +any symbols it references. This may result in those symbols getting 
>> discarded 
>> +by GCC as unreferenced.

We can omit "by GCC" here.

>> +While Basic @code{asm} blocks are implicitly volatile, they are not treated 
>> +as though they used a "memory" clobber (@pxref{Clobbers}).

What does it mean for a block to be volatile, as opposed to a variable?

Should this be ``memory'' (note the quotes)?

>> -@section Assembler Instructions with C Expression Operands
>> +@subsection Extended Asm - Assembler Instructions with C Expression Operands
>> +@cindex @code{asm} keyword
>>  @cindex extended @code{asm}
>> -@cindex @code{asm} expressions
>>  @cindex assembler instructions
>> -@cindex registers
>>  
>> -In an assembler instruction using @code{asm}, you can specify the
>> -operands of the instruction using C expressions.  This means you need not
>> -guess which registers or memory locations contain the data you want
>> -to use.
>> +The @code{asm} keyword allows you to embed assembler instructions within C 
>> +code. With Extended @code{asm} you can read and write C variables from 
>> +assembler and include jumps from assembler code to C labels.

>> +@ifhtml
>> +asm [volatile] ( AssemblerTemplate : [OutputOperands] [ : [InputOperands] [ 
>> : [Clobbers] ] ] )
>>  
>> -For example, here is how to use the 68881's @code{fsinx} instruction:
>> +asm [volatile] goto ( AssemblerTemplate : : [InputOperands] : [Clobbers] : 
>> GotoLabels )
>> +@end ifhtml
>> +@ifnothtml
>> +asm [volatile] ( AssemblerTemplate 
>> +                 : [OutputOperands] 
>> +                 [ : [InputOperands] 
>> +                 [ : [Clobbers] ] ])

Why does the @ifhtml variant have a "goto" when the other does not?

>> +By definition, Extended @code{asm} is an @code{asm} statement that contains 
>> +operands. To separate the classes of operands, you use colons. Basic 
>> +@code{asm} statements contain no colons. (So, for example, 
>> +@code{asm("int $3")} is Basic @code{asm}, and @code{asm("int $3" : )} is 
>> +Extended @code{asm}. @pxref{Basic Asm}.)

What does "classes of operands refer to"?  This does not seem to be
evident from the context.

>> +@subsubheading Remarks
>> +The @code{asm} statement allows you to include assembly instructions 
>> directly 
>> +within C code. This may help you to maximize performance in time-sensitive 
>> +code or to access assembly instructions that are not readily available to C 
>> +programs.

This feels like it comes rather late in the flow.  Wouldn't that be
a comment pretty early on in this section?

>> +Note that Extended @code{asm} statements must be inside a function. Only 
>> +Basic @code{asm} may be outside functions (@pxref{Basic Asm}).

We had a similary paragraph earlier on.  Perhaps only keep this where
it relates to Extended statements?  Not sure about this, though.

>> +While the uses of @code{asm} are many and varied, it may help to think of 
>> an 
>> +@code{asm} statement as a series of low-level instructions that convert 
>> input 
>> +parameters to output parameters. So a simple (if not particularly useful) 
>> +example for i386 using @code{asm} might look like this:

This, without the example possibly, would have been good earlier, when
all the different classes where described.

>> +GCC's optimizer sometimes discards @code{asm} statements if it determines 
>> +that it has no need for the output variables.

How about just saying "GCC", not referring to the optimizer here?

>> +This i386 code demonstrates a case that does not use (or require) the 
>> +@code{volatile} qualifier. If it is performing assertion checking, this 
>> code 
>> +uses @code{asm} to perform the validation.

>> +@code{DoCheck} routine. By omitting the @code{volatile} qualifier when it 
>> +isn't needed you allow the optimizer to produce the most efficient code 
>> +possible.

How about "Only including ...when it is really necessary allows the 
optimizer", that is, a more positive ("included" instead of "omitting") 
approach?

>> +GCC's optimizer will not treat this code like the non-volatile code in the 
>> +earlier examples. It does not move it out of loops or omit it on the 
>> +assumption that the result from a previous call is still valid.

"GCC will..."

>> +Under certain circumstances, GCC may duplicate (or remove duplicates of) 
>> your 
>> +asm code as part of optimization. This can lead to unexpected duplicate 
>> symbol 

@code{asm}

And did we not have the same (or very similar) statement a bit earlier?

>> +directives. The GCC compiler does not parse the assembler instructions 

Just "GCC".

>> +You may place multiple assembler instructions together in a single 
>> @code{asm} 
>> +string, separated by the characters normally used in assembly code for the 
>> +system. A combination that works in most places is a newline to break the 
>> +line, plus a tab character to move to the instruction field (written as 
>> +"\n\t"). Some assemblers allow semicolons as a line separator. However, 
>> note 
>> +that some assembler dialects use semicolons to start a comment. 

This also looks quite familiar.  Is there a way to build up and avoid
the redundancy?

>> +GCC may support multiple assembler dialects (such as "att" or "intel") for 
>> +inline assembler. The list of supported dialects depends on the 
>> implementation 
>> +details of the specific build of the compiler. When writing assembler, be 
>> +aware of which dialect is the compiler's default. Assembler code that works 
>> +correctly when compiled using one dialect will likely fail if compiled 
>> using 
>> +another.

How about saying "When writing assembler code", since the term
assembler is a bit ambigous?

>> +@example
>> +@{ dialect0 | dialect1 | dialect2... @}
>> +@end example
:
>> +This construct outputs 'dialect0' when using dialect #0 to compile the 
>> code, 
>> +'dialect1' for dialect #1, etc. If there are fewer alternatives within the 
>> +braces than the number of dialects the compiler supports, the construct 
>> +outputs nothing.

How does the user know what is dialect #0?  Same for the others?

>> +There is no support for nesting dialect alternatives. Also, there is no 
>> +"escape" for an open brace (@{), so do not use open braces in an Extended 
>> +@code{asm} template other than as a dialect indicator.

``escape'' (note the quotes)

>> +In addition to the tokens described by the input, output, and goto 
>> operands, there are a few special cases:

How about clobbers?

>> +@itemize
>> +@item
>> +'%%' outputs a single '%' into the assembler code.

>> +@item
>> +'%=' outputs a number that is unique to each instance of the asm statement 
>> +in the entire compilation. This option is useful when creating local labels 
>> +and referring to them multiple times in a single template that generates 
>> +multiple assembler instructions. 

@code{asm} statement

And again the quotes don't seem right (in a few cases here).

>> +Output constraints must begin with either "=" (a variable overwriting an 
>> +existing value) or "+" (when reading and writing). When using "=", do not 
>> +assume the location will contain the existing value (except when tying the 
>> +variable to an input; @pxref{InputOperands,,Input Operands}).

@code{=}

@code{+}

Possibly under proper quotes as well?

>> +After the prefix, there must be one or more additional constraints 
>> +(@pxref{Constraints}) that describe where the value resides. Common 
>> +constraints include "r" for register, "m" for memory, and "i" for 
>> immediate. 

Similarly here.

>> +control to select the specific register you want, Local Reg Vars may 
>> provide 
>> +a solution (@pxref{Local Reg Vars}).

Local Reg Vars should be properly expanded, I assume.

>> +Specifies the C variable name of the output (enclosed by parenthesis).

"parentheses" (plural)

>> Accepts 
>> +any (non-constant) variable within scope.

What's a non-constant variable?  Or just a constant variable?

> > +@var{b}. Combining the '@code{&}' constraint with the register constraint 
> > +ensures that modifying @var{a} will not affect what address is referenced 
> > by 
> > +@var{b}. Omitting the '@code{&}' constraint means that the location of 
> > @var{b} 

If we use qoutes, they should read `...' in a couple of cases here.

>> +Input constraints must be a string containing one or more constraints 
>> +(@pxref{Constraints}). When you give more than one possible constraint 
>> +(for example, @code{"irm"}), the compiler will choose the most efficient 

I don't think  irm  should be under quotes.  The entire @code{...} I
could understand better.

>> +either "=" or "+". If you must use a specific register, but your Machine

Quotes.

>> +Constraints do not provide sufficient control to select the specific 
>> +register you want, Local Reg Vars may provide a solution 
>> +(@pxref{Local Reg Vars}).

Can we use a better term than Local Reg Vars?  Or is that totally
entrenched?

>> +Input constraints can also be digits (for example, @code{"0"}). This 
>> indicates 

Quotes inside of @code do not seem approriate here.

                                                              
>> +Remarks:
>> +
>> +The total number of input + output + goto operands has a limit of 30.

We also had this data point earlier in the text of this patch already,
didn't we?

>> +constraint @code{"0"} for input operand 1 says that it must occupy the same 

Mind the quotes.

>> +The "cc" clobber indicates that the assembler code modifies the flags 

Quotes.  And @code{cc} I guess.

>> +hardware register; "cc" serves to name this register. On other machines, 
>> +condition code handling is different, and specifying "cc" has no effect. 
>> But 
>> +it is valid no matter what the machine.

Two more.

>> +The "memory" clobber tells the compiler that the assembly code performs 
>> memory

Same here.

>> +reads or writes to items other than those listed in the input and output > 
>> > +specific register values to memory before executing the asm. Further, the 

@asm

>> +compiler will not assume that any values read from memory before the 
>> +@code{asm} will remain unchanged after the @code{asm}; it will reload them 
>> as 
>> +needed. This effectively forms a read/write memory barrier for the compiler.

"before an @code{asm}...after that @code{asm}"

>> +Note that this clobber does not prevent the @emph{processor} from doing 
>> +speculative reads past the @code{asm} statement. To stop that, you need 
>> +processor-specific fence instructions.

"prevent" instead of "stop" (since with "stop" it would have already
started).

>> +An @code{asm goto} statement may not have outputs (which means that the 
>> +statement is implicitly volatile). This is due to an internal restriction 
>> in 
>> +the compiler: that control transfer instructions cannot have outputs. If 
>> the 

I'd say "of the compiler" and omit "that" in "that control transfer...".

>> +assembler code does modify anything, use the "memory" clobber to force the 

``memory''

>> +To reference a label, prefix it with @code{%l} followed by a number. This 
>> +number is zero-based and includes any input arguments (for example, if the 
>> +@code{asm} has three inputs and references two labels, refer to the first 
>> +label as @code{%l3} and the second as @code{%l4}).

I understood this one, mostly based on the example, and am wondering
how we can improve it.

Something like "@code{%l} followed by its position in GotoLabels
(starting with zero) plus the number of input arguments."

And then "For example,..." as an independent sentence, not in
parentheses.

>> +Input, output, and goto operands for extended @code{asm} can use modifiers 
>> to 
>> +affect the code output to the assembler. For example, the following code 
>> uses 
>> +the 'h' and 'b' modifiers for i386:

extended @code{asm} statements

Mind the quotes.

>> +@multitable {Modifier} {Print the opcode suffix for the size of th} 
>> {Operand} {masm=att} {masm=intel}

WHere does th come from here?

Should this be @var{th}?

>> +@headitem Modifier @tab Description @tab Operand @tab masm=att @tab 
>> masm=intel

@code{masm...} ?

>> +@tab Print the opcode suffix for the size of the current integer 
>> operand (one of b/w/l/q).

@code{b}/@code{w}/....

> > +@tab Print the QImode name for a "high" register.

``high''

>> +Some targets require that GCC track the size of each instruction used,
>> +in order to generate correct code.

I'd omit the comma here.

>> +instruction supported by that processor.  (When working out the number
>> +of instructions, it assumes that any occurrence of a newline or of
>> +whatever statement separator character is supported by the assembler --
>> +typically @samp{;} -- indicates the end of an instruction.)

--- instead of --

>> +Normally, GCC's estimate is perfectly adequate to ensure that correct

"adequate", without "perfectly".  That'd be an odd combination. :-)

>> +code is generated, but it is possible to confuse the compiler if you use
>> +pseudo instructions or assembler macros that expand into multiple real
>> +instructions, or if you use assembler directives that expand to more
>> +space in the object file than is needed for a single instruction.
>> +If this happens then the assembler produces a diagnostic saying that
>> +a label is unreachable.

"produces" -> "may produce" ?

>> +Sometimes when writing inline asm code, you need to make an operand be a 

@code{asm}


Hurray, and that's it for now.

If you send an updated patch, I'd appreciate if you could also just
send a diff between the file as it is now (after the current patch
applied) and after those updates so that we can see what just changed
in between the two reviews.

As I wrote above, I approve if you make these changes -- or reply
noting where and why you did not -- and Andrew and Richard ack.


Thank you for doing this in the first place, and being persistent
in getting this applied.  This surely has been a lot, I means a _lot_,
of effort!

Gerald

Reply via email to