Remove the bit-field by mrb_value.tt #871

Merged
merged 1 commit into from Mar 30, 2013

Conversation

Projects
None yet
5 participants
Contributor

monaka commented Feb 22, 2013

This patch is based on discussion with @miura1729.

I think the bit-field by mrb_value.tt is redundant.
It make increase CPU cost. And less memory merit. Maybe (as it depends on ABI).

Contributor

masuidrive commented Feb 24, 2013

This bit-field was for less memory.
But it isn't to work correctly.
if you want enum is 8 bits, you need to add extra attribute.

#include <stdio.h>
enum e1 { foo, bar, buzz };
struct s1 { enum e1 ee; };
struct s2 { enum e1 ee:8; };
struct s3 { enum e1 ee:8; } __attribute__((packed));

int main(int argc, char **argv) {
  printf("%d, %d, %d\n",sizeof(struct s1), sizeof(struct s2), sizeof(struct s3));
  return 0;
}

=> 4, 4, 1

But it isn't C99 standard.

I think we can add extra configuration ENABLE_PACKED_STRUCT.
I'll make this patch tomorrow. please review it.

Contributor

monaka commented Feb 24, 2013

Hi, @masuidrive.

It's less merit to add compiler dependent patches.

We got just 3bytes compression per one mrb_value.
You might think it will great compression since there are a lot fo mrb_values in the runtime.
But all mrb_values are aligned.
The align size will be more than sizeof(int). (Strictly it depends on the target ABI. But almost all RISCs will be.)

Contributor

monaka commented Feb 24, 2013

The first commit to use tt:8 is b0e94d8.
The aim of the patch is to modify type of tt uint8_t to enum, right?

And, was it really required to use bit-field? I can't catch behind it.

Contributor

monaka commented Feb 25, 2013

Additional information to 871#issuecomment-14010473.

#include <stdio.h>
#include <stdint.h>
#include <inttypes.h>

enum e1 { foo, bar, buzz };
struct s3 { double a; enum e1 ee:8; } __attribute__((packed));

int main(int argc, char **argv) {
  struct s3 a;
  struct s3 b;
  struct s3 c;

  printf("%zu, %"PRIdPTR", %"PRIdPTR"\n", sizeof(struct s3), (intptr_t)&c - (intptr_t)&b, (intptr_t)&b - (intptr_t)&a);
  return 0;
}

i686-apple-darwin10-gcc 4.2.1 (Apple Inc. build 5666) (dot 3)

9, -16, -16

arm-pizzafactory-eabi-gcc 4.6.0 (experimental)

zu, 9, -12

(Showing "zu" is caused by a bug in Newlib)

We might get 9 as alignments on some targets like m32r, powerpc.

Anyway, alignments are completely ABI dependent.
I suggest we should choice portability rather than low memory usage.

Contributor

masamitsu-murase commented Feb 25, 2013

I think that bit-field is not used to reduce mrb_value but is used to reduce the size of MRB_OBJECT_HEADER in value.h.

In this file, MRB_OBJECT_HEADER is defined as follows:

#define MRB_OBJECT_HEADER \
  enum mrb_vtype tt:8;\
  unsigned int color:3;\
  unsigned int flags:21;\
  struct RClass *c;\
  struct RBasic *gcnext

In this case, tt, color and flags are expected to be packed to 32bit on many ABIs.

I think that we should refer not only to mrb_value but also to MRB_OBJECT_HEADER.

Contributor

masamitsu-murase commented Feb 25, 2013

I'm not so sure about bit-field but I think that some compilers warn when enum mrb_vtype tt in mrb_value is set to enum mrb_vtype tt:8 in MRB_OBJECT_HEADER, don't they?

Contributor

mattn commented Feb 25, 2013

I'm guessing behavior of bit field are depend on compilers. So I sugguest to replace it to accurate types.

typedef char mrb_type
Contributor

monaka commented Feb 25, 2013

Roughly telling C99 spec.
enum is nearly equivalent to int (but has a different signedness rule).
And enum with bit-fields is also nearly equivalent to int (this also has a yet another different signedness rule).
So we can substitute bit-fields for enum. Also enum for bit-fields.
(Of course the value must be in bit-width. In this case, 8bits.)

Strictly speaking, we can't expect whether tt:8 in MRB_OBJECT_HEADER is packed.
But I also feel that it will be packed same as @masamitsu-murase says.
So I suggest we leave MRB_OBJECT_HEADER as is.

Meanwhile, I can't say any merits to declare mrb_value.tt with bit-fields.

Contributor

monaka commented Feb 25, 2013

It might be also a solution to use char same as @mattn says. The similar solution is to revert to uint8_t.
But it's effective to suppress bugs by using enum.
So I support using enum (if it isn't with redundant bit-fields).

Contributor

masamitsu-murase commented Feb 25, 2013

Thanks for your explanation. :-)

Contributor

monaka commented Mar 9, 2013

I got warning "warning: type of bit-field ‘tt’ is a GCC extension" with -pedantic option.

It's reasonable. It is described in C99 6.7.2.1.
"A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type."

It may accept enum-bit-field not only gcc but also MSVC, Clang. But it is not C99 conformance.

monaka referenced this pull request Mar 13, 2013

Closed

Remove bit fields. #972

@matz matz added a commit that referenced this pull request Mar 30, 2013

@matz matz Merge pull request #871 from monaka/pr-remove-bit-field-in-mrb_value
Remove the bit-field by mrb_value.tt
f475da9

@matz matz merged commit f475da9 into mruby:master Mar 30, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment