Bug 153668 - Air needs documentation
Summary: Air needs documentation
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-29 13:30 PST by Filip Pizlo
Modified: 2016-05-31 14:38 PDT (History)
1 user (show)

See Also:


Attachments
the patch (22.38 KB, patch)
2016-05-31 13:59 PDT, Filip Pizlo
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-01-29 13:30:10 PST
Patch coming soon, hopefully.
Comment 1 Filip Pizlo 2016-05-31 13:59:44 PDT
Created attachment 280178 [details]
the patch
Comment 2 Mark Lam 2016-05-31 14:24:38 PDT
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/
Comment 3 Filip Pizlo 2016-05-31 14:26:55 PDT
(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".
Comment 4 Saam Barati 2016-05-31 14:27:37 PDT
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
Comment 5 Benjamin Poulain 2016-05-31 14:34:01 PDT
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.
Comment 6 Filip Pizlo 2016-05-31 14:37:31 PDT
(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.
Comment 7 Filip Pizlo 2016-05-31 14:38:05 PDT
(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.