Bug 159863 – RegisterSet should use a Bitmap instead of a BitVector so that it never allocates memory and is trivial to copy

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 283872 [details]
the patch

[WebKit Commit Bot]

Attachment 283872 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/RegisterSet.h:146:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 283874 [details]
the patch

fix arms

[WebKit Commit Bot]

Attachment 283874 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/RegisterSet.h:146:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 283874 [details]
the patch

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

r=me

> Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:40
> +    static const unsigned numGPRs = 32;

Why put this in the MacroAssembler instead of GPRInfo? I thought GPRInfo is the place for such things.

> Source/WTF/ChangeLog:8
> +        Give Bigmap all of the power of BitVector (except for automatic resizing). This means a

Typo: Bigmap

> Source/WTF/wtf/Bitmap.h:39
> +// NOTE: WordType must not be bigger than unsigned.

This should be a static assert instead of a comment.

[Filip Pizlo]

(In reply to comment #5)
> Comment on attachment 283874 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283874&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARM64.h:40
> > +    static const unsigned numGPRs = 32;
> 
> Why put this in the MacroAssembler instead of GPRInfo? I thought GPRInfo is
> the place for such things.

No, because GPRInfo is the set of registers that are useful to high-level-MacroAssembler-based compilers.  It already has a numberOfRegisters field, and it doesn't count things like SP, FP, or the scratch registers.

That's the separation: MacroAssembler has some reflection over registers that tells you about *all* registers, while GPRInfo has reflection over registers that tells you just about those registers that you would find useful if you were casually using MacroAssembler API.

Note that RegisterSet is to MacroAssembler as TempRegisterSet is to GPRInfo/FPRInfo.  TempRegisterSet uses GPRInfo::numberOfRegisters for example.

> 
> > Source/WTF/ChangeLog:8
> > +        Give Bigmap all of the power of BitVector (except for automatic resizing). This means a
> 
> Typo: Bigmap

Fixed.

> 
> > Source/WTF/wtf/Bitmap.h:39
> > +// NOTE: WordType must not be bigger than unsigned.
> 
> This should be a static assert instead of a comment.

OK.

[Filip Pizlo]

Created attachment 283876 [details]
patch for landing

[WebKit Commit Bot]

Attachment 283876 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/RegisterSet.h:146:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

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

[Csaba Osztrogonác]

(In reply to comment #9)
> Landed in https://trac.webkit.org/changeset/203365

This one or r203364 made many tests crash on Apple Mac 32 bit bots:
- https://build.webkit.org/builders/Apple%20Yosemite%2032-bit%20JSC%20%28BuildAndTest%29/builds/9824
- https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29/builds/2948

( Last known good revision: r203363, first known bad revision: r203368 )

[Csaba Osztrogonác]

This change is innocent, the other one is the culprit.