Bug 170742

Summary:

B3 -O1 should not allocateStackByGraphColoring

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam, ysuzuki

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
laid the foundation	none
it is written	none
seems to work	none
getting closer to done	none
so close	none
the patch	keith_miller: review+

Filip Pizlo

Reported 2017-04-11 12:06:55 PDT

Patch forthcoming.

Attachments
laid the foundation (31.03 KB, patch) 2017-04-11 12:07 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
it is written (97.25 KB, patch) 2017-04-11 13:06 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
seems to work (85.72 KB, patch) 2017-04-11 14:59 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
getting closer to done (108.68 KB, patch) 2017-04-11 16:53 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
so close (124.46 KB, patch) 2017-04-11 16:57 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (124.46 KB, patch) 2017-04-11 17:08 PDT, Filip Pizlo	keith_miller: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2017-04-11 12:07:32 PDT

Created attachment 306836 [details] laid the foundation

Filip Pizlo

Comment 2 2017-04-11 13:06:45 PDT

Created attachment 306842 [details] it is written I haven't even compiled it yet.

Filip Pizlo

Comment 3 2017-04-11 14:59:03 PDT

Created attachment 306858 [details] seems to work

Filip Pizlo

Comment 4 2017-04-11 16:53:06 PDT

Created attachment 306874 [details] getting closer to done

Filip Pizlo

Comment 5 2017-04-11 16:57:54 PDT

Created attachment 306875 [details] so close

Build Bot

Comment 6 2017-04-11 17:00:25 PDT

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

Comment 7 2017-04-11 17:08:11 PDT

Created attachment 306877 [details] the patch

Build Bot

Comment 8 2017-04-11 17:09:47 PDT

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 9 2017-04-12 09:51:06 PDT

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

Comment 10 2017-04-12 11:30:09 PDT

(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

Comment 11 2017-04-12 14:38:07 PDT

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

Saam Barati

Comment 12 2017-04-12 20:51:40 PDT

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?

Note You need to log in before you can comment on or make changes to this bug.