Bug 176336 – Make the distinction between entrypoints and CFG roots more clear by naming things better

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

I think this will make the code easier cleaner, instead of just having comments stating that a particular field is only used in SSA.

[Saam Barati]

The vector called m_entrypoints now I'm going to rename to m_roots. This represents the CFG roots. I'll also rename m_entrypointToArguments to m_rootToArguments. Similarly, I'll rename Graph::isEntrypoint to Graph::isRoot

m_numberOfEntrypoints is only used in SSA, and I'll keep the same name. That field represents the number of logical entrypoints this compilation is making. It's not necessarily the number of CFG roots.

[Saam Barati]

Created attachment 319930 [details]
patch

[Mark Lam]

Comment on attachment 319930 [details]
patch

r=me

[Keith Miller]

Comment on attachment 319930 [details]
patch

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

r=me too.

> Source/JavaScriptCore/ChangeLog:17
> +

You should probably say that you renamed m_entrypoints to m_roots in the changelog.

[Michael Saboff]

Comment on attachment 319930 [details]
patch

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

r=me.  The code changes seem fine.  Please update the ChangeLog.

> Source/JavaScriptCore/ChangeLog:16
> +        Graph::m_numberOfEntrypoints is only used in SSA when compiling with
> +        EntrySwitch. It represents the logical number of entrypoints the compilation
> +        will end up with. Crucially, Graph::m_numberOfEntrypoints != Graph::m_roots.size().
> +        This is the main reason I've renamed the fields and related methods.

I don't find changes to m_numberOfEntrypoints in the patch.  It would be good to discuss the reasoning behind the change of m_entrypointToArguments to m_rootToArguments.

[Saam Barati]

Comment on attachment 319930 [details]
patch

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

>> Source/JavaScriptCore/ChangeLog:16
>> +        This is the main reason I've renamed the fields and related methods.
> 
> I don't find changes to m_numberOfEntrypoints in the patch.  It would be good to discuss the reasoning behind the change of m_entrypointToArguments to m_rootToArguments.

This patch doesn't change m_numberOfEntrypoints on purpose. That field is retaining its name because it describes its purpose.

Also, this patch renames m_entrypointToArguments to m_rootToArguments to be consistent with the change of m_entrypoints to m_roots.

>> Source/JavaScriptCore/ChangeLog:17
>> +
> 
> You should probably say that you renamed m_entrypoints to m_roots in the changelog.

It says this above.

[Saam Barati]

New changelog:

This patch does renaming to make the distinction between Graph::m_entrypoints
and Graph::m_numberOfEntrypoints more clear. The source of confusion is that
Graph::m_entrypoints.size() is not equivalent to Graph::m_numberOfEntrypoints.
Graph::m_entrypoints is really just the CFG roots. In CPS, this vector has
size >= 1. In SSA, the size is always 1. This patch renames Graph::m_entrypoints
to Graph::m_roots. To be consistent, this patch also renames Graph's m_entrypointToArguments
field to m_rootToArguments.

Graph::m_numberOfEntrypoints retains its name. This field is only used in SSA
when compiling with EntrySwitch. It represents the logical number of entrypoints
the compilation will end up with. Each EntrySwitch has m_numberOfEntrypoints
cases.

[Saam Barati]

Created attachment 319932 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 319932 [details]
patch for landing

Clearing flags on attachment: 319932

Committed r221637: <http://trac.webkit.org/changeset/221637>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34693633>