Bug 183416 - Make it possible to randomize register allocation
Summary: Make it possible to randomize register allocation
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: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-07 14:30 PST by Filip Pizlo
Modified: 2018-03-12 10:08 PDT (History)
11 users (show)

See Also:


Attachments
the patch (6.44 KB, patch)
2018-03-07 14:31 PST, Filip Pizlo
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-03-07 14:30:32 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2018-03-07 14:31:36 PST
Created attachment 335224 [details]
the patch
Comment 2 EWS Watchlist 2018-03-07 14:36:12 PST
Attachment 335224 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirCode.cpp:79:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/b3/air/AirCode.cpp:80:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Keith Miller 2018-03-07 14:41:30 PST
Comment on attachment 335224 [details]
the patch

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

> Source/JavaScriptCore/b3/air/AirCode.cpp:62
> +            Vector<Reg> volatileRegs;

Why not call this callerSaveRegs? Is it to avoid typo bugs?

> Source/JavaScriptCore/b3/air/AirCode.cpp:65
>              all.exclude(RegisterSet::stackRegisters());

I think the wasm bug might be that you are not excluding the pinned registers?
Comment 4 Keith Miller 2018-03-07 14:43:09 PST
Comment on attachment 335224 [details]
the patch

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

>> Source/JavaScriptCore/b3/air/AirCode.cpp:65
>>              all.exclude(RegisterSet::stackRegisters());
> 
> I think the wasm bug might be that you are not excluding the pinned registers?

Nvm, I don't think this is the issue. Ignore me.
Comment 5 Keith Miller 2018-03-07 14:43:37 PST
r=me.
Comment 6 Filip Pizlo 2018-03-08 09:03:07 PST
(In reply to Keith Miller from comment #3)
> Comment on attachment 335224 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335224&action=review
> 
> > Source/JavaScriptCore/b3/air/AirCode.cpp:62
> > +            Vector<Reg> volatileRegs;
> 
> Why not call this callerSaveRegs? Is it to avoid typo bugs?

Yeah.  I don't really know what the best terminology is, but I have often used "volatile" versus "callee-save", because it's easier to distinguish the two.

> 
> > Source/JavaScriptCore/b3/air/AirCode.cpp:65
> >              all.exclude(RegisterSet::stackRegisters());
> 
> I think the wasm bug might be that you are not excluding the pinned
> registers?

Nah.  This is just about order.  The pinned regs will get excluded anyway.
Comment 7 Filip Pizlo 2018-03-08 09:12:16 PST
Landed in https://trac.webkit.org/changeset/229412/webkit
Comment 8 Radar WebKit Bug Importer 2018-03-08 09:30:59 PST
<rdar://problem/38266059>
Comment 9 Darin Adler 2018-03-10 17:40:39 PST
Comment on attachment 335224 [details]
the patch

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

> Source/WTF/wtf/MathExtras.h:534
> +template<typename VectorType, typename RandomFunc>
> +void shuffleVector(VectorType& vector, size_t size, const RandomFunc& randomFunc)
> +{
> +    for (size_t i = 0; i + 1 < size; ++i)
> +        std::swap(vector[i], vector[i + randomFunc(size - i)]);
> +}

Why doesn’t this use std::random_shuffle?
Comment 10 Keith Miller 2018-03-12 10:08:49 PDT
(In reply to Darin Adler from comment #9)
> Comment on attachment 335224 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335224&action=review
> 
> > Source/WTF/wtf/MathExtras.h:534
> > +template<typename VectorType, typename RandomFunc>
> > +void shuffleVector(VectorType& vector, size_t size, const RandomFunc& randomFunc)
> > +{
> > +    for (size_t i = 0; i + 1 < size; ++i)
> > +        std::swap(vector[i], vector[i + randomFunc(size - i)]);
> > +}
> 
> Why doesn’t this use std::random_shuffle?

According to http://en.cppreference.com/w/cpp/algorithm/random_shuffle, std::random_shuffle is deprecated in C++14 and removed in C++17 so it probably doesn't make sense to use it. We could use std::shuffle but that has a really obnoxious API...