Bug 170742 – B3 -O1 should not allocateStackByGraphColoring

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 306836 [details]
laid the foundation

[Filip Pizlo]

Created attachment 306842 [details]
it is written

I haven't even compiled it yet.

[Filip Pizlo]

Created attachment 306858 [details]
seems to work

[Filip Pizlo]

Created attachment 306874 [details]
getting closer to done

[Filip Pizlo]

Created attachment 306875 [details]
so close

[Build Bot]

Attachment 306875 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:463:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:463:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 306877 [details]
the patch

[Build Bot]

Attachment 306877 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:463:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:463:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 306877 [details]
the patch

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

r=me.

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:307
> +            [&] (Tmp, TmpData& data) -> bool {

Why pass the Tmp if you don't use it anywhere?

[Filip Pizlo]

(In reply to Keith Miller from comment #9)
> Comment on attachment 306877 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306877&action=review
> 
> r=me.
> 
> > Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:307
> > +            [&] (Tmp, TmpData& data) -> bool {
> 
> Why pass the Tmp if you don't use it anywhere?

Good point, I'll remove!

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/215292/webkit

[Saam Barati]

Comment on attachment 306877 [details]
the patch

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

> Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackByLinearScan.cpp:584
> +            ptrdiff_t offset = -static_cast<ptrdiff_t>(m_code.frameSize()) - static_cast<ptrdiff_t>(entry.spillIndex) * 8 - 8;

Why is the -8 needed? Also, is it worth using sizeof(void*) instead of 8?