Bug 183416 – Make it possible to randomize register allocation

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 335224 [details]
the patch

[EWS Watchlist]

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.

[Keith Miller]

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?

[Keith Miller]

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.

[Keith Miller]

r=me.

[Filip Pizlo]

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

[Filip Pizlo]

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

[Radar WebKit Bug Importer]

<rdar://problem/38266059>

[Darin Adler]

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?

[Keith Miller]

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