Replace C++ bitfields with our own BitFields

Replace C++ bitfields with our own BitFields

Shave this yak from orbit, it's the only way to be sure.

BUG=chromium:427616
LOG=n
R=svenpanne@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=25148

[Jakob Kummerow, 2014-11-04 21:45:21]

jkummerow@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Jakob Kummerow, 2014-11-04 21:45:21]

As discussed.

Some of these changes should not affect object layout at all; in some other
cases I took the opportunity to move more things into the bitfield. No change in
behavior is intended, except possibly in situations where the compiler wasn't
doing what we thought it was doing.

There are two more occurrences of "_ : [0-9]" in src/compiler/ which I've left
alone for now; other than that the entire codebase should now be uniformly using
BitField<...> for packing fields.

Please take a look.

[brucedawson, 2014-11-05 01:33:17]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2014-11-05 01:33:18]

I looked through all of the .h files and some of the .cc files. All of the
changes appear good (although it would be very easy to miss an error). Some of
the changes look like they will reduce the size of structures.

Some of the changes are avoidable (I commented on some but not all of these). If
you have a set of bit fields where all of the items are bool then should will be
fine on all compilers and arguably should be left alone to minimize churn/risk.

Similarly if you have a bit field where all of the items are int or unsigned int
or other 32-bit types then all is well.

It is only when mixing sizes (int and bool for instance) or using enums in bit
fields that things go awry.

Even some of the enum cases could be solved more simply by just not using a bit
field. There are some cases where bit fields are being used where simple typed
enums with no bit fields would also work.

I would recommend only using the BitFields class where it offers a benefit over
normal bit fields and over just avoiding bit fields entirely.

https://codereview.chromium.org/700963002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/700963002/diff/1/src/ast.h#newcode427
src/ast.h:427: uint32_t bit_field_;
Why use a bit field at all? byte/bool/bool take up three bytes, pads to four
bytes, which should be perfectly space efficient, more run-time efficient.

https://codereview.chromium.org/700963002/diff/1/src/heap-snapshot-generator.h
File src/heap-snapshot-generator.h (left):

https://codereview.chromium.org/700963002/diff/1/src/heap-snapshot-generator....
src/heap-snapshot-generator.h:56: int from_index_ : 29;
FWIW, I believe that this usage of bit fields is portable. Certainly VC++
handles it fine.

https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):

https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1530: bool map_is_stable_ : 1;
Good fix. VC++ would handle this type of bit field poorly -- the map_is_stable_
member would be in a separate backing store so that they would use 64 bits
instead of 32.

https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:1900: bool done_with_replay_ : 1;
Also a good fix.

https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:6960: StoreFieldOrKeyedMode store_mode_ : 1;
Good fix. Could also use separate fields and a typed enum.

https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h#...
src/hydrogen-instructions.h:7175: StoreFieldOrKeyedMode store_mode_: 1;
Could also fix with two non-bit field bools and a typed enum.

https://codereview.chromium.org/700963002/diff/1/src/profile-generator.h
File src/profile-generator.h (left):

https://codereview.chromium.org/700963002/diff/1/src/profile-generator.h#oldc...
src/profile-generator.h:92: Builtins::Name builtin_id_ : 8;
Could presumably use typed enums (unsigned char) and no bit fields.

https://codereview.chromium.org/700963002/diff/1/src/unicode.h
File src/unicode.h (left):

https://codereview.chromium.org/700963002/diff/1/src/unicode.h#oldcode40
src/unicode.h:40: bool value_ : 1;
Good fix. VC++ cannot handle this unless value_ is also stored in a 32-bit type
(unsigned int).

[Jakob Kummerow, 2014-11-05 09:02:04]

Thanks for looking. As I wrote before, I'm aware that some of the changes are
unnecessary in the sense that the old code could be expected to work well. The
idea was to be both explicit about intended object layout, and consistent in how
it's achieved: we're using BitField<...> in a bunch of places anyway, and with
this CL it's now *the* canonical way to pack less-than-pointer-sized fields
together.

So I'd prefer to keep all the changes, as opposed to undoing some or using typed
enum classes in some cases, unless someone feels strongly about it.

[Sven Panne, 2014-11-05 09:11:49]

NOT LGTM as it is: ast.h must stay as it is, and the changes in hydrogen.h are
obfuscating code. Consistency is not really a strong argument, readability and
space efficiency where it matters is. :-) I think there are only 2 valid use
cases for BitField:

   * We need exact control over the layout because we access the data from
generated machine code and can't rely on any ABI conventions.

   * We can't get a sane layout where it matters without fighting VC layout
rules and nonsensical GCC warnings.

https://codereview.chromium.org/700963002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/700963002/diff/1/src/ast.h#newcode427
src/ast.h:427: uint32_t bit_field_;
On 2014/11/05 01:33:17, brucedawson wrote:
> Why use a bit field at all? byte/bool/bool take up three bytes, pads to four
> bytes, which should be perfectly space efficient, more run-time efficient.

All the code in ast.h *must* stay as it is, for the reason see:
https://code.google.com/p/chromium/issues/detail?id=417697. This part of the CL
would basically revert all the goodness already done.

Quick explanation: The layout of the AST nodes is carefully tuned for space
efficiency because asm.js code results in *huge* ASTs. As it is,
to_boolean_types consumes one byte, is_parenthesized and is_multiparenthesized
go into the next byte. The important thing: The most common node VariableProxy
starts with 3 1bit fields, which go into the next byte. (With clang at least,
use "clang++ -c -Xclang -fdump-record-layouts" to see what really happens)

If we blindingly change the layout here, we will blow away hundreds of megabytes
for asm.js stuff. Yes, this is all very fragile, but it's important
nevertheless.

Perhaps we should have a look at the layout of the nodes under VC, but I don't
know how to do that. Hints? :-)

https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h#newcode222
src/hydrogen.h:222: uint32_t bit_field_;
Can we leave this as it is? It's only a sequence of booleans, which is OK, and
furthermore HBasicBlock is not relevant for any kind of space savings. The
BitField stuff is only obfuscating things here.

https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h#newcode1716
src/hydrogen.h:1716: uint32_t bit_field_;
Same here: This is obfuscating things, and IfBuilder is not relevant for space
efficiency.

https://codereview.chromium.org/700963002/diff/1/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/700963002/diff/1/src/scanner.cc#newcode249
src/scanner.cc:249: if (c0_ < 0) break;
How is this related to the bitfield fiddling?

https://codereview.chromium.org/700963002/diff/1/src/scanner.cc#newcode631
src/scanner.cc:631: token = Token::EOS;
Same here...

[Jakob Kummerow, 2014-11-05 12:13:46]

PTAL.

https://codereview.chromium.org/700963002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/700963002/diff/1/src/ast.h#newcode427
src/ast.h:427: uint32_t bit_field_;
On 2014/11/05 09:11:49, Sven Panne wrote:
> On 2014/11/05 01:33:17, brucedawson wrote:
> > Why use a bit field at all? byte/bool/bool take up three bytes, pads to four
> > bytes, which should be perfectly space efficient, more run-time efficient.
> 
> All the code in ast.h *must* stay as it is, for the reason see:
> https://code.google.com/p/chromium/issues/detail?id=417697. This part of the
CL
> would basically revert all the goodness already done.
> 
> Quick explanation: The layout of the AST nodes is carefully tuned for space
> efficiency because asm.js code results in *huge* ASTs. As it is,
> to_boolean_types consumes one byte, is_parenthesized and is_multiparenthesized
> go into the next byte. The important thing: The most common node VariableProxy
> starts with 3 1bit fields, which go into the next byte. (With clang at least,
> use "clang++ -c -Xclang -fdump-record-layouts" to see what really happens)
> 
> If we blindingly change the layout here, we will blow away hundreds of
megabytes
> for asm.js stuff. Yes, this is all very fragile, but it's important
> nevertheless.
> 
> Perhaps we should have a look at the layout of the nodes under VC, but I don't
> know how to do that. Hints? :-)

Used a smaller field per offline discussion.

https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h#newcode222
src/hydrogen.h:222: uint32_t bit_field_;
On 2014/11/05 09:11:49, Sven Panne wrote:
> Can we leave this as it is? It's only a sequence of booleans, which is OK, and
> furthermore HBasicBlock is not relevant for any kind of space savings. The
> BitField stuff is only obfuscating things here.

Done. (Though I don't agree with the term "obfuscating"; yes it's more verbose,
but also more consistent and explicit/reliable which IMHO justifies the change.
But whatever.)

https://codereview.chromium.org/700963002/diff/1/src/hydrogen.h#newcode1716
src/hydrogen.h:1716: uint32_t bit_field_;
On 2014/11/05 09:11:49, Sven Panne wrote:
> Same here: This is obfuscating things, and IfBuilder is not relevant for space
> efficiency.

Done.

https://codereview.chromium.org/700963002/diff/1/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/700963002/diff/1/src/scanner.cc#newcode249
src/scanner.cc:249: if (c0_ < 0) break;
On 2014/11/05 09:11:49, Sven Panne wrote:
> How is this related to the bitfield fiddling?

As the comment says, the unicode_cache_ only accepts unsigned inputs. Since it's
now using BitField, it trips over a DCHECK that the implicitly-converted -1
(which signals EOS) can actually be stored.

[Sven Panne, 2014-11-05 12:23:20]

lgtm

[Jakob Kummerow, 2014-11-05 12:41:15]

Committed patchset #2 (id:20001) manually as 25148 (presubmit successful).

[Nico, 2015-10-23 00:24:11]

Zombie review comment! Can you expand on the "as discussed" bit a bit? The BUG=
line is just some generic tracking bug, and the CL description doesn't really
say what's happening here. I think it was because bool bitfields are considered
signed and aren't packed tightly with neighboring unsigned bitfields, but it'd
be good if the commit description or the review had some more detail from the
author what the intent was :-)

[Jakob Kummerow, 2015-10-23 12:27:00]

On 2015/10/23 00:24:11, Nico (vacation Fri Oct 23) wrote:
> Zombie review comment! Can you expand on the "as discussed" bit a bit? The
BUG=
> line is just some generic tracking bug, and the CL description doesn't really
> say what's happening here. I think it was because bool bitfields are
considered
> signed and aren't packed tightly with neighboring unsigned bitfields, but it'd
> be good if the commit description or the review had some more detail from the
> author what the intent was :-)

This CL replaced https://codereview.chromium.org/694003002/ which contained more
background. (You're right, of course, that the justification should have been
copied over.)

In short: 1-bit bitfields for 2-value enums (like "enum Foo { kBar, kBaz}; Foo
foo_ : 1;") are a correctness issue in VC++.

[brucedawson, 2015-10-23 13:54:41]

Yup, 1-bit enum bitfields are a problem because enums are signed in VC++. 1-bit
int bitfields would show the same problem in gcc - a bitfield that can't store
'1' (0 and -1 only) is confusing.

Additionally the packing rules for bitfields can vary between compilers. In
particular, in VC++ different sized bitfield storage types don't pack together,
so bool doesn't pack with short or int, int doesn't pack with short, etc.

What a mess.

The initial problem was found by VC++'s /analyze which pointed out that we were
comparing one of the bitfields to '1', which is a value it could never have.