Do BLKmode bit-fields still exist?

[Michael Matz]

Hello,

while working on factoring out very old code (expand_assignment,
store_expr, store_field) I stumbled over the above question.  There's code
all over the compiler that tries to handle BLKmode bit-field FIELD_DECLs
in a certain way, but I can't for the life of me construct anything that
actually results in such fields and I meanwhile assume that over the years
we simply can't generate them anymore.

In the various bit-field accessors we sometimes use VOIDmode to mark an
access to a real bit-field (otherwise we wouldn't be able to differ
between an byte-aligned bit-field from a normal field, when looking at
only bitpos + bitlength).  I'm not talking about that.  I'm specifically
talking about bit-field FIELD_DECLs with DECL_MODE == BLKmode.

From the code that tries to handle these it seems that this once meant an
"unaligned bit-field", which doesn't really make sense (we can handle all
situations and combinations of bitofs+bitlength in generic code).  The
handling in store_field is especially bogus, it tries to handle the case
where the target (being a register) is aligned, the bit-field unaligned,
and goes over memory for this.  That's bollocks, we can do nice bit-magic
for registers, however "aligned" the bit pattern is.

Trying to trace where we could possibly construct such field decls we are
often careful to not store BLKmode into DECL_MODE of field decls.  The
only place where we could get BLKmode is if the TYPE_MODE of the field
decls type is BLKmode.

Now, theoretically we can get TYPE_MODE == BLKmode very easily.  But not
for types from which bit-fields can be constructed.  I'm pretty sure that
we can construct bit field FIELD_DECLs only for integer types.  All
targets always have QImode through TImode available (in terms of
machmode.def, some targets explicitely disallow using e.g. TImode).  So
all integer types that a user can write have a non-BLKmode.  And that mode
is used as the DECL_MODE for the bit field FIELD_DECL, no matter how
large (depending on the language, excess size will give an error or round
down to the max size of the underlying type).

Sometimes we're also using mode_for_size to set DECL_MODEs of bit-fields
(indirectly through types), but for bit field sizes that actually can be
constructed we always have a mode available.

Hence, I don't see how we ever can construct a BLKmode bit-field
FIELD_DECL.

In a desparate try to get some testcases which do have BLKmode bit-fields
I bootstrapped and regtested the below patch (as part of a larger patch,
though) on seven architectures with all languages (on two without Ada).  
To no avail.

I tried to directly construct testcases which would possibly generate
BLKmode at least for architectures which have very limited bitwidth (AVR),
ala:

typedef unsigned int TIint __attribute__((mode(DI)));
struct Unaligned{
    int a:7;
    TIint b:63;
    int c:8;
}__attribute__((packed));

and reading/storing into the fields, varying the mode, the bitsizes and
the like.  To no avail again.

Can somebody else come up with a testcase for his pet-target that triggers
the gcc_unreachables() in the patch?  Pretty please?


Ciao,
Michael.
Index: expr.c
===================================================================
--- expr.c (revision 153935)
+++ expr.c (working copy)
@@ -5795,6 +5887,7 @@ store_field (rtx target, HOST_WIDE_INT b
       if (bitsize != (HOST_WIDE_INT) GET_MODE_BITSIZE (GET_MODE (target)))
  emit_move_insn (object, target);
 
+      gcc_unreachable ();
       store_field (blk_object, bitsize, bitpos, mode, exp, type, alias_set,
    nontemporal);
 
@@ -5979,7 +6012,10 @@ get_inner_reference (tree exp, HOST_WIDE
       if (!DECL_BIT_FIELD (field))
  mode = DECL_MODE (field);
       else if (DECL_MODE (field) == BLKmode)
- blkmode_bitfield = true;
+ {
+  blkmode_bitfield = true;
+  gcc_unreachable ();
+ }
 
       *punsignedp = DECL_UNSIGNED (field);
     }

[Jeff Law]

On 11/05/09 10:02, Michael Matz wrote:

> Hello,
>
> while working on factoring out very old code (expand_assignment,
> store_expr, store_field) I stumbled over the above question.  There's code
> all over the compiler that tries to handle BLKmode bit-field FIELD_DECLs
> in a certain way, but I can't for the life of me construct anything that
> actually results in such fields and I meanwhile assume that over the years
> we simply can't generate them anymore.
>
> In the various bit-field accessors we sometimes use VOIDmode to mark an
> access to a real bit-field (otherwise we wouldn't be able to differ
> between an byte-aligned bit-field from a normal field, when looking at
> only bitpos + bitlength).  I'm not talking about that.  I'm specifically
> talking about bit-field FIELD_DECLs with DECL_MODE == BLKmode.
>
>  From the code that tries to handle these it seems that this once meant an
> "unaligned bit-field", which doesn't really make sense (we can handle all
> situations and combinations of bitofs+bitlength in generic code).  The
> handling in store_field is especially bogus, it tries to handle the case
> where the target (being a register) is aligned, the bit-field unaligned,
> and goes over memory for this.  That's bollocks, we can do nice bit-magic
> for registers, however "aligned" the bit pattern is.
>
> Trying to trace where we could possibly construct such field decls we are
> often careful to not store BLKmode into DECL_MODE of field decls.  The
> only place where we could get BLKmode is if the TYPE_MODE of the field
> decls type is BLKmode.
>
> Now, theoretically we can get TYPE_MODE == BLKmode very easily.  But not
> for types from which bit-fields can be constructed.  I'm pretty sure that
> we can construct bit field FIELD_DECLs only for integer types.  All
> targets always have QImode through TImode available (in terms of
> machmode.def, some targets explicitely disallow using e.g. TImode).  So
> all integer types that a user can write have a non-BLKmode.  And that mode
> is used as the DECL_MODE for the bit field FIELD_DECL, no matter how
> large (depending on the language, excess size will give an error or round
> down to the max size of the underlying type).
>
> Sometimes we're also using mode_for_size to set DECL_MODEs of bit-fields
> (indirectly through types), but for bit field sizes that actually can be
> constructed we always have a mode available.
>
> Hence, I don't see how we ever can construct a BLKmode bit-field
> FIELD_DECL.
>
> In a desparate try to get some testcases which do have BLKmode bit-fields
> I bootstrapped and regtested the below patch (as part of a larger patch,
> though) on seven architectures with all languages (on two without Ada).
> To no avail.
>
> I tried to directly construct testcases which would possibly generate
> BLKmode at least for architectures which have very limited bitwidth (AVR),
> ala:
>
> typedef unsigned int TIint __attribute__((mode(DI)));
> struct Unaligned{
>      int a:7;
>      TIint b:63;
>      int c:8;
> }__attribute__((packed));
>
> and reading/storing into the fields, varying the mode, the bitsizes and
> the like.  To no avail again.
>
> Can somebody else come up with a testcase for his pet-target that triggers
> the gcc_unreachables() in the patch?  Pretty please?
>    

Perhaps the PA when returning small unaligned structures?   Hmm,
probably not, I think we just needed to support copying BLKmode objects
to/from registers to make that work.    Nevermind.

jeff

[Eric Botcazou-3]

> In a desparate try to get some testcases which do have BLKmode bit-fields
> I bootstrapped and regtested the below patch (as part of a larger patch,
> though) on seven architectures with all languages (on two without Ada).

Yet it's easy in Ada on platforms with strict alignment, e.g. SPARC:

package P is

  type Rec1 is record
    I1 : Integer;
    I2 : Integer;
    I3 : Integer;
  end record;

  type R2 is record
    B : Boolean;
    R : Rec1;
  end record;
  pragma Pack (R2);

end P;

(gdb) p debug_tree(0x2aaaaab2bdc0)
 <field_decl 0x2aaaaab2bdc0 r
    type <record_type 0x2aaaaabc64d0 p__rec1 sizes-gimplified visited BLK
        size <integer_cst 0x2aaaaab27f90 constant visited 96>
        unit size <integer_cst 0x2aaaaabd7d20 constant visited 12>
        align 32 symtab 0 alias set -1 canonical type 0x2aaaaabc64d0
        fields <field_decl 0x2aaaaab2bb40 i1 type <integer_type 0x2aaaaabc6580
integer>
            nonaddressable SI file p.ads line 4 col 5
            size <integer_cst 0x2aaaaaaf1420 constant visited 32>
            unit size <integer_cst 0x2aaaaaaf1090 constant visited 4>
            align 32 offset_align 64
            offset <integer_cst 0x2aaaaaaf1b40 constant visited 0>
            bit offset <integer_cst 0x2aaaaaaf1b70 constant 0> context
<record_type 0x2aaaaabc64d0 p__rec1> chain <field_decl 0x2aaaaab2bbe0 i2>>
Ada size <integer_cst 0x2aaaaab27f90 96>
        reference_to_this <reference_type 0x2aaaaabc6630> chain <type_decl
0x2aaaaab34540 p__rec1>>
    external packed bit-field BLK file p.ads line 11 col 5 size <integer_cst
0x2aaaaab27f90 96> unit size <integer_cst 0x2aaaaabd7d20 12>
    align 8 offset_align 64 offset <integer_cst 0x2aaaaaaf1b40 0>
    bit offset <integer_cst 0x2aaaaaaf1180 type <integer_type 0x2aaaaab010b0
bit_size_type> constant visited 8> bit_field_type <record_type 0x2aaaaabc64d0
p__rec1> context <record_type 0x2aaaaabc6790 p__r2>>

We set DECL_BIT_FIELD in the front-end because the field is misaligned.

--
Eric Botcazou

[Richard Guenther-2]

On Fri, Nov 6, 2009 at 10:46 AM, Eric Botcazou <ebotcazou@...> wrote:

>> In a desparate try to get some testcases which do have BLKmode bit-fields
>> I bootstrapped and regtested the below patch (as part of a larger patch,
>> though) on seven architectures with all languages (on two without Ada).
>
> Yet it's easy in Ada on platforms with strict alignment, e.g. SPARC:
>
> package P is
>
>  type Rec1 is record
>    I1 : Integer;
>    I2 : Integer;
>    I3 : Integer;
>  end record;
>
>  type R2 is record
>    B : Boolean;
>    R : Rec1;
>  end record;
>  pragma Pack (R2);
>
> end P;
>
> (gdb) p debug_tree(0x2aaaaab2bdc0)
>  <field_decl 0x2aaaaab2bdc0 r
>    type <record_type 0x2aaaaabc64d0 p__rec1 sizes-gimplified visited BLK
>        size <integer_cst 0x2aaaaab27f90 constant visited 96>
>        unit size <integer_cst 0x2aaaaabd7d20 constant visited 12>
>        align 32 symtab 0 alias set -1 canonical type 0x2aaaaabc64d0
>        fields <field_decl 0x2aaaaab2bb40 i1 type <integer_type 0x2aaaaabc6580
> integer>
>            nonaddressable SI file p.ads line 4 col 5
>            size <integer_cst 0x2aaaaaaf1420 constant visited 32>
>            unit size <integer_cst 0x2aaaaaaf1090 constant visited 4>
>            align 32 offset_align 64
>            offset <integer_cst 0x2aaaaaaf1b40 constant visited 0>
>            bit offset <integer_cst 0x2aaaaaaf1b70 constant 0> context
> <record_type 0x2aaaaabc64d0 p__rec1> chain <field_decl 0x2aaaaab2bbe0 i2>>
> Ada size <integer_cst 0x2aaaaab27f90 96>
>        reference_to_this <reference_type 0x2aaaaabc6630> chain <type_decl
> 0x2aaaaab34540 p__rec1>>
>    external packed bit-field BLK file p.ads line 11 col 5 size <integer_cst
> 0x2aaaaab27f90 96> unit size <integer_cst 0x2aaaaabd7d20 12>
>    align 8 offset_align 64 offset <integer_cst 0x2aaaaaaf1b40 0>
>    bit offset <integer_cst 0x2aaaaaaf1180 type <integer_type 0x2aaaaab010b0
> bit_size_type> constant visited 8> bit_field_type <record_type 0x2aaaaabc64d0
> p__rec1> context <record_type 0x2aaaaabc6790 p__r2>>
>
> We set DECL_BIT_FIELD in the front-end because the field is misaligned.

Isn't it enough to specify DECL_PACKED here?  The tree.h docs
about DECL_BIT_FIELD are a bit unspecific compared to
DECL_PACKED.

/* Nonzero in a FIELD_DECL means it is a bit field, and must be
accessed   specially.  */

vs.

/* In a FIELD_DECL, indicates this field should be bit-packed.  */

where it seems, as your field isn't a bitfield, using DECL_PACKED
looks more appropriate?

Richard.

> --
> Eric Botcazou
>

[Eric Botcazou-3]

> Isn't it enough to specify DECL_PACKED here?  The tree.h docs
> about DECL_BIT_FIELD are a bit unspecific compared to
> DECL_PACKED.
>
> /* Nonzero in a FIELD_DECL means it is a bit field, and must be
> accessed   specially.  */
>
> vs.
>
> /* In a FIELD_DECL, indicates this field should be bit-packed.  */
>
> where it seems, as your field isn't a bitfield, using DECL_PACKED
> looks more appropriate?

No, DECL_PACKED is an "input" flag for stor-layout.c, it isn't used in the
middle-end (except in the recently added contains_packed_reference but this
is wrong and should be fixed), the "output" flag that drives the middle-end
is DECL_BIT_FIELD.  stor-layout.c attempts to clear the latter to improve the
code, it never changes the former.

--
Eric Botcazou

[Michael Matz]

Hi,

On Fri, 6 Nov 2009, Eric Botcazou wrote:

> Yet it's easy in Ada on platforms with strict alignment, e.g. SPARC:
>
> package P is
>
>   type Rec1 is record
>     I1 : Integer;
>     I2 : Integer;
>     I3 : Integer;
>   end record;
>
>   type R2 is record
>     B : Boolean;
>     R : Rec1;
>   end record;
>   pragma Pack (R2);
>
> end P;
>
> (gdb) p debug_tree(0x2aaaaab2bdc0)
>  <field_decl 0x2aaaaab2bdc0 r
>     type <record_type 0x2aaaaabc64d0 p__rec1 sizes-gimplified visited BLK
>     external packed bit-field BLK file p.ads line 11 col 5 size <integer_cst
>
> We set DECL_BIT_FIELD in the front-end because the field is misaligned.

Uahhh!  A bitfield of RECORD_TYPE!  Marvelous.

Molding this into a testcase that actually writes into some parts, like:

procedure Fields is
  type Rec1 is record
    I1 : Integer;
  end record;

  type R2 is record
    B : Boolean;
    R : Rec1;
  end record;
  pragma Pack (R2);

  r,rr : R2;
  subr,subr2 : Rec1;
  procedure useme (x:in out R2;y:in out Rec1);
  pragma Import (C, useme);
  function giveme return Integer;
  pragma Import (c,giveme);
begin
  subr2.I1 := giveme;
  subr := subr2;
  r.R := subr2;
  rr := r;
  useme(rr, subr);
end;

triggers the gcc_unreachable in get_inner_references (on sparc), okay.  
But I can't trigger the one in store_field, because the target for these
stores will alway be a non-register due to the unaligned fields in there,
hmm. (the above needs -O1 -fno-tree-ccp -fno-tree-copy-prop -fno-tree-sra
ot not forward the giveme() result but retain the store into r.R).


Ciao,
Michael.