Bug 170108 – Air should use RegisterSet for RegLiveness

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

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 305428 [details]
maybe it'll work

[Filip Pizlo]

Created attachment 305434 [details]
the patch

[Build Bot]

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.

[Yusuke Suzuki]

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.

[Filip Pizlo]

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

[Saam Barati]

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?

[Saam Barati]

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

[Filip Pizlo]

(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.