Bug 170108

Summary: Air should use RegisterSet for RegLiveness
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, sbarati, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
maybe it'll work
none
the patch ysuzuki: review+

Description Filip Pizlo 2017-03-26 11:51:01 PDT
We shouldn't use those fancy index sparse sets and vectors and stuff.  We should just use RegisterSet, since that's a soooper fast and compact bitvector.
Comment 1 Filip Pizlo 2017-03-26 11:52:00 PDT
Created attachment 305428 [details]
maybe it'll work
Comment 2 Filip Pizlo 2017-03-26 13:32:30 PDT
Created attachment 305434 [details]
the patch
Comment 3 Build Bot 2017-03-26 13:34:33 PDT
Attachment 305434 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/B3TimingScope.cpp:60:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLowerAfterRegAlloc.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.h:45:  The parameter name "code" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirReportUsedRegisters.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:52:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:61:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:64:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:68:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:79:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:86:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:104:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:114:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 12 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Yusuke Suzuki 2017-03-26 13:59:34 PDT
Comment on attachment 305434 [details]
the patch

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

r=me

> Source/JavaScriptCore/b3/B3TimingScope.cpp:60
> +    return ensurePointer(s_state, [] { return new State(); });

Nice thing!

> Source/JavaScriptCore/b3/air/AirLiveness.h:-403
> -typedef AbstractLiveness<RegLivenessAdapter> RegLiveness;

OK, this will be replaced with specialized RegLiveness, which uses RegisterSet.

> Source/JavaScriptCore/b3/air/AirRegLiveness.cpp:95
> +}

OK, it is (logically) identical to the AbstractLiveness one.
Comment 5 Filip Pizlo 2017-03-26 14:41:28 PDT
Landed in https://trac.webkit.org/changeset/214408/webkit
Comment 6 Saam Barati 2017-03-26 15:49:42 PDT
Comment on attachment 305434 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        AbstractLiveness<>, but it's about 30% faster. It gets its speed boost from just using

Awesome!

> Source/WTF/wtf/Atomics.h:532
> +        T* newValue = func();

Style nit: It feels weird to take a lambda here, and assume that it calls "new" on T, since you call "delete" below. Why not just take arguments for T's constructor that you forward to it?
Comment 7 Saam Barati 2017-03-26 15:50:28 PDT
Comment on attachment 305434 [details]
the patch

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

>> Source/WTF/wtf/Atomics.h:532
>> +        T* newValue = func();
> 
> Style nit: It feels weird to take a lambda here, and assume that it calls "new" on T, since you call "delete" below. Why not just take arguments for T's constructor that you forward to it?

Or perhaps have two lambdas. One for making a value, one for cleanup
Comment 8 Filip Pizlo 2017-03-26 16:36:40 PDT
(In reply to Saam Barati from comment #7)
> Comment on attachment 305434 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305434&action=review
> 
> >> Source/WTF/wtf/Atomics.h:532
> >> +        T* newValue = func();
> > 
> > Style nit: It feels weird to take a lambda here, and assume that it calls "new" on T, since you call "delete" below. Why not just take arguments for T's constructor that you forward to it?
> 
> Or perhaps have two lambdas. One for making a value, one for cleanup

You're right, the lambda is dumb.  It should just take a type parameter and new it, and then it should take an argument pack to pass to the constructor.