Bug 170742 - B3 -O1 should not allocateStackByGraphColoring
Summary: B3 -O1 should not allocateStackByGraphColoring
Status: RESOLVED FIXED
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: 2017-04-11 12:06 PDT by Filip Pizlo
Modified: 2017-04-12 20:51 PDT (History)
8 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-04-11 12:06:55 PDT 
Patch forthcoming.
Comment 1 Filip Pizlo 2017-04-11 12:07:32 PDT 
Created attachment 306836 [details]
laid the foundation
Comment 2 Filip Pizlo 2017-04-11 13:06:45 PDT 
Created attachment 306842 [details]
it is written

I haven't even compiled it yet.
Comment 3 Filip Pizlo 2017-04-11 14:59:03 PDT 
Created attachment 306858 [details]
seems to work
Comment 4 Filip Pizlo 2017-04-11 16:53:06 PDT 
Created attachment 306874 [details]
getting closer to done
Comment 5 Filip Pizlo 2017-04-11 16:57:54 PDT 
Created attachment 306875 [details]
so close
Comment 6 Build Bot 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.
Comment 7 Filip Pizlo 2017-04-11 17:08:11 PDT 
Created attachment 306877 [details]
the patch
Comment 8 Build Bot 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.
Comment 9 Keith Miller 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?
Comment 10 Filip Pizlo 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!
Comment 11 Filip Pizlo 2017-04-12 14:38:07 PDT 
Landed in https://trac.webkit.org/changeset/215292/webkit
Comment 12 Saam Barati 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?