Bug 153668 – Air needs documentation

• Status: NEW
• Component: JavaScriptCore

[Filip Pizlo]

Patch coming soon, hopefully.

[Filip Pizlo]

Created attachment 280178 [details]
the patch

[Mark Lam]

Comment on attachment 280178 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280178&action=review

r=me.  Please fix typos.

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:45
> +    <p>B3 is designed to be protable to many kinds of CPUs. Currently, it supports x86-64 and ARM64,

/protable/portable/

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:189
> +      liveness and interference analyses reason about the

/analyses/analysis/

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 280178 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280178&action=review
> 
> r=me.  Please fix typos.
> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:45
> > +    <p>B3 is designed to be protable to many kinds of CPUs. Currently, it supports x86-64 and ARM64,
> 
> /protable/portable/
> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:189
> > +      liveness and interference analyses reason about the
> 
> /analyses/analysis/

"Liveness and interference analyses" are two analyses, and "analyses" is the plural of "analysis".

[Saam Barati]

Comment on attachment 280178 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280178&action=review

LGTM

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:156
> +        access is for performance (either <i>warm</i> or <i>cold</i>), and how writes affect the top

might be worth another sentence explaining warm/cold

[Benjamin Poulain]

View in context: https://bugs.webkit.org/attachment.cgi?id=280178&action=review

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:28
> +      <a href="http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/b3/air/AirBasicBlock.h"><code>Inst</code></a>s.

Wrong link to AirInst

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:45
> +    <p>B3 is designed to be protable to many kinds of CPUs. Currently, it supports x86-64 and ARM64,

Typo: protable

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:61
> +      and abstract stack slots.</p>

No link to AirStackSlot?

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:154
> +      <dd>The role of an argument is an enum that describes the timing of when the argument is

"timing" is a bit meta here, one could think its related to scheduling. Maybe "order" would be clearer?

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:341
> +      CPUs, such as X86 or X86-64.</p>

Lower case "x" on x86 && x86-64.

[Filip Pizlo]

(In reply to comment #4)
> Comment on attachment 280178 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280178&action=review
> 
> LGTM
> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:156
> > +        access is for performance (either <i>warm</i> or <i>cold</i>), and how writes affect the top
> 
> might be worth another sentence explaining warm/cold

Done.

[Filip Pizlo]

(In reply to comment #5)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280178&action=review
> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:28
> > +      <a href="http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/b3/air/AirBasicBlock.h"><code>Inst</code></a>s.
> 
> Wrong link to AirInst

Fixed.

> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:45
> > +    <p>B3 is designed to be protable to many kinds of CPUs. Currently, it supports x86-64 and ARM64,
> 
> Typo: protable

Fixed.

> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:61
> > +      and abstract stack slots.</p>
> 
> No link to AirStackSlot?

Added a link.

> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:154
> > +      <dd>The role of an argument is an enum that describes the timing of when the argument is
> 
> "timing" is a bit meta here, one could think its related to scheduling.
> Maybe "order" would be clearer?

I'll keep "timing" for now.  The word "order" doesn't work as well in some of the rest of the text.

> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:341
> > +      CPUs, such as X86 or X86-64.</p>
> 
> Lower case "x" on x86 && x86-64.

Fixed.