Re: [PATCH] fix arm neon ICE by widening tree_type's precision field

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Jun 9, 2009 at 12:31 PM, Nathan Froyd<froydnj@codesourcery.com> wrote:
> On Tue, Jun 09, 2009 at 11:45:09AM -0400, Richard Guenther wrote:
>> >> Richard, Steven, do either of you have complaints about moving the
>> >> lang_flag_* fields from type into base (renaming them into
>> >> type_lang_flag_*) to free up space instead of moving packed_flag?
>> >
>> > I don't like it but there doesn't seem to be a better alternative. All
>> > the TYPE_LANG_FLAG_x flags are in use, and so are all other bits (I
>> > actually checked that). ?Someone already suspiciously squeezed out a
>> > bit from TYPE_MODE.
>> >
>> > So if moving the type.lang_flag bits to tree_base is what you end up
>> > doing, can you please also give one bit back to TYPE_MODE so that it
>> > is 8 bit again just like all other ENUM_BITFIELD(machine_mode) (only
>> > recog has 16 bits for machine mode -- will we ever need >256 modes?),
>> > and reshuffle the bitfields a bit, to make the access to the
>> > most-often used fields cheaper?
>>
>> I don't like moving all lang-specific type flags to base. ?There are some
>> bits in base used for non-generic things already, so moving a bit like
>> user_align there should be following existing practice, like
>> saturating_flag which is type-only as well (it could even
>> re-use like default_def_flag).
>
> OK, how about this slightly more aggressive patch, which:
>
> - Moves packed_flag and user_align into tree_base;
>
> - Frees enough space in tree_type to widen precision and mode;
>
> - Aligns tree_type.mode on a byte boundary;
>
> - Changes DECL_PACKED to use tree_base.packed_flag instead of some
> ?random flag in tree_decl_common; and
>
> - Renumbers tree_decl_common fields and uses appropriately.
>
> LTO folks, this is probably going to break a few things in the next
> merge. ?Sorry about that.
>
> Sanity-checked on arm-none-linux-gnueabi; will run bootstrap and testing
> on x86_64-unknown-linux-gnu. ?OK to commit?

Works for me.  Ok after 24h if it bootstraps/regtests ok to let other people
have time to comment.

Thanks,
Richard.

> -Nathan
>
> 2009-06-09 ?Nathan Froyd ?<froydnj@codesourcery.com>
>
> ? ? ? ?* tree.h (tree_base): Add packed_flag and user_align fields.
> ? ? ? ?Decrease size of spare field.
> ? ? ? ?(TYPE_USER_ALIGN): Use user_align from tree_base.
> ? ? ? ?(DECL_USER_ALIGN): Likewise.
> ? ? ? ?(TYPE_PACKED): Use packed_flag from tree_base.
> ? ? ? ?(DECL_PACKED): Likewise.
> ? ? ? ?(tree_type): Delete packed_flag and user_align fields. ?Widen
> ? ? ? ?precision field. ?Widen mode field and shuffle fields to align
> ? ? ? ?mode on an 8-bit boundary.
> ? ? ? ?(tree_decl_common): Delete decl_flag_1 and user_align fields.
> ? ? ? ?Renumber decl_flag_* fields. ?Fix comments. ?Widen
> ? ? ? ?decl_common_unused field.
> ? ? ? ?(DECL_HAS_VALUE_EXPR_P): Adjust for renumbering of decl_flag_*
> ? ? ? ?fields.
> ? ? ? ?(DECL_EXTERNAL): Likewise.
> ? ? ? ?(DECL_BIT_FIELD): Likewise.
> ? ? ? ?(DECL_NONADDRESSABLE_P): Likewise.
> ? ? ? ?(TYPE_DECL_SUPRESS_DEBUG): Likewise.
> ? ? ? ?* config/arm/arm-modes.def (XImode): Make it an INT_MODE.
>
> Index: config/arm/arm-modes.def
> ===================================================================
> --- config/arm/arm-modes.def ? ?(revision 148302)
> +++ config/arm/arm-modes.def ? ?(working copy)
> @@ -62,6 +62,4 @@ VECTOR_MODES (FLOAT, 16); ? ? /* ? ? ? V
> ?INT_MODE (EI, 24);
> ?INT_MODE (OI, 32);
> ?INT_MODE (CI, 48);
> -/* ??? This should actually have 512 bits but the precision only has 9
> - ? bits. ?*/
> -FRACTIONAL_INT_MODE (XI, 511, 64);
> +INT_MODE (XI, 64);
> Index: testsuite/ChangeLog
> ===================================================================
> --- testsuite/ChangeLog (revision 148302)
> +++ testsuite/ChangeLog (working copy)
> @@ -1,3 +1,7 @@
> +2009-06-09 ?Nathan Froyd ?<froydnj@codesourcery.com>
> +
> + ? ? ? * gcc.target/arm/neon-dse-1.c: New test.
> +
> ?2009-06-08 ?Jan Hubicka ?<jh@suse.cz>
>
> ? ? ? ?PR debug/39834
> Index: testsuite/gcc.target/arm/neon-dse-1.c
> ===================================================================
> --- testsuite/gcc.target/arm/neon-dse-1.c ? ? ? (revision 0)
> +++ testsuite/gcc.target/arm/neon-dse-1.c ? ? ? (revision 0)
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target arm_neon_ok } */
> +/* { dg-options "-O1" } */
> +/* { dg-add-options arm_neon } */
> +
> +#include <arm_neon.h>
> +
> +void neon_internal_error(int *dst, int *src)
> +{
> + ?uint16x8x4_t sval;
> +
> + ?sval = vld4q_u16((void *)src);
> + ?vst4q_u16((void *)dst,sval);
> +}
> Index: tree.h
> ===================================================================
> --- tree.h ? ? ?(revision 148302)
> +++ tree.h ? ? ?(working copy)
> @@ -373,8 +373,10 @@ struct GTY(()) tree_base {
> ? unsigned lang_flag_6 : 1;
>
> ? unsigned visited : 1;
> + ?unsigned packed_flag : 1;
> + ?unsigned user_align : 1;
>
> - ?unsigned spare : 23;
> + ?unsigned spare : 21;
>
> ? union tree_ann_d *ann;
> ?};
> @@ -2108,7 +2110,7 @@ extern enum machine_mode vector_type_mod
>
> ?/* 1 if the alignment for this type was requested by "aligned" attribute,
> ? ?0 if it is the default for this type. ?*/
> -#define TYPE_USER_ALIGN(NODE) (TYPE_CHECK (NODE)->type.user_align)
> +#define TYPE_USER_ALIGN(NODE) (TYPE_CHECK (NODE)->common.base.user_align)
>
> ?/* The alignment for NODE, in bytes. ?*/
> ?#define TYPE_ALIGN_UNIT(NODE) (TYPE_ALIGN (NODE) / BITS_PER_UNIT)
> @@ -2219,7 +2221,7 @@ extern enum machine_mode vector_type_mod
>
> ?/* Indicated that objects of this type should be laid out in as
> ? ?compact a way as possible. ?*/
> -#define TYPE_PACKED(NODE) (TYPE_CHECK (NODE)->type.packed_flag)
> +#define TYPE_PACKED(NODE) (TYPE_CHECK (NODE)->common.base.packed_flag)
>
> ?/* Used by type_contains_placeholder_p to avoid recomputation.
> ? ?Values are: 0 (unknown), 1 (false), 2 (true). ?Never access
> @@ -2237,17 +2239,16 @@ struct GTY(()) tree_type {
> ? tree attributes;
> ? unsigned int uid;
>
> - ?unsigned int precision : 9;
> - ?ENUM_BITFIELD(machine_mode) mode : 7;
> -
> - ?unsigned string_flag : 1;
> + ?unsigned int precision : 10;
> ? unsigned no_force_blk_flag : 1;
> ? unsigned needs_constructing_flag : 1;
> ? unsigned transparent_union_flag : 1;
> - ?unsigned packed_flag : 1;
> ? unsigned restrict_flag : 1;
> ? unsigned contains_placeholder_bits : 2;
>
> + ?ENUM_BITFIELD(machine_mode) mode : 8;
> +
> + ?unsigned string_flag : 1;
> ? unsigned lang_flag_0 : 1;
> ? unsigned lang_flag_1 : 1;
> ? unsigned lang_flag_2 : 1;
> @@ -2255,7 +2256,6 @@ struct GTY(()) tree_type {
> ? unsigned lang_flag_4 : 1;
> ? unsigned lang_flag_5 : 1;
> ? unsigned lang_flag_6 : 1;
> - ?unsigned user_align : 1;
>
> ? unsigned int align;
> ? alias_set_type alias_set;
> @@ -2509,7 +2509,7 @@ struct GTY(()) tree_decl_minimal {
> ?#define DECL_ALIGN_UNIT(NODE) (DECL_ALIGN (NODE) / BITS_PER_UNIT)
> ?/* Set if the alignment of this DECL has been set by the user, for
> ? ?example with an 'aligned' attribute. ?*/
> -#define DECL_USER_ALIGN(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.user_align)
> +#define DECL_USER_ALIGN(NODE) (DECL_COMMON_CHECK (NODE)->common.base.user_align)
> ?/* Holds the machine mode corresponding to the declaration of a variable or
> ? ?field. ?Always equal to TYPE_MODE (TREE_TYPE (decl)) except for a
> ? ?FIELD_DECL. ?*/
> @@ -2546,7 +2546,7 @@ struct GTY(()) tree_decl_minimal {
> ? ?example, for a FUNCTION_DECL, DECL_SAVED_TREE may be non-NULL and
> ? ?DECL_EXTERNAL may be true simultaneously; that can be the case for
> ? ?a C99 "extern inline" function. ?*/
> -#define DECL_EXTERNAL(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.decl_flag_2)
> +#define DECL_EXTERNAL(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.decl_flag_1)
>
> ?/* Nonzero in a ..._DECL means this variable is ref'd from a nested function.
> ? ?For VAR_DECL nodes, PARM_DECL nodes, and FUNCTION_DECL nodes.
> @@ -2615,7 +2615,6 @@ struct GTY(()) tree_decl_common {
> ? unsigned ignored_flag : 1;
> ? unsigned abstract_flag : 1;
> ? unsigned artificial_flag : 1;
> - ?unsigned user_align : 1;
> ? unsigned preserve_flag: 1;
> ? unsigned debug_expr_is_from : 1;
>
> @@ -2631,22 +2630,20 @@ struct GTY(()) tree_decl_common {
> ? /* In LABEL_DECL, this is DECL_ERROR_ISSUED.
> ? ? ?In VAR_DECL and PARM_DECL, this is DECL_REGISTER. ?*/
> ? unsigned decl_flag_0 : 1;
> - ?/* In FIELD_DECL, this is DECL_PACKED. ?*/
> - ?unsigned decl_flag_1 : 1;
> ? /* In FIELD_DECL, this is DECL_BIT_FIELD
> ? ? ?In VAR_DECL and FUNCTION_DECL, this is DECL_EXTERNAL.
> - ? ? In TYPE_DECL, this is TYPE_DECL_SUPRESS_DEBUG. ?*/
> - ?unsigned decl_flag_2 : 1;
> + ? ? In TYPE_DECL, this is TYPE_DECL_SUPPRESS_DEBUG. ?*/
> + ?unsigned decl_flag_1 : 1;
> ? /* In FIELD_DECL, this is DECL_NONADDRESSABLE_P
> - ? ? In VAR_DECL and PARM_DECL, this is DECL_HAS_VALUE_EXPR. ?*/
> - ?unsigned decl_flag_3 : 1;
> + ? ? In VAR_DECL and PARM_DECL, this is DECL_HAS_VALUE_EXPR_P. ?*/
> + ?unsigned decl_flag_2 : 1;
> ? /* Logically, these two would go in a theoretical base shared by var and
> ? ? ?parm decl. */
> ? unsigned gimple_reg_flag : 1;
> ? /* In VAR_DECL, PARM_DECL and RESULT_DECL, this is DECL_BY_REFERENCE. ?*/
> ? unsigned decl_by_reference_flag : 1;
> ? /* Padding so that 'off_align' can be on a 32-bit boundary. ?*/
> - ?unsigned decl_common_unused : 2;
> + ?unsigned decl_common_unused : 4;
>
> ? /* DECL_OFFSET_ALIGN, used only for FIELD_DECLs. ?*/
> ? unsigned int off_align : 8;
> @@ -2672,7 +2669,7 @@ extern void decl_value_expr_insert (tree
> ? ?decl itself. ?This should only be used for debugging; once this field has
> ? ?been set, the decl itself may not legitimately appear in the function. ?*/
> ?#define DECL_HAS_VALUE_EXPR_P(NODE) \
> - ?(TREE_CHECK2 (NODE, VAR_DECL, PARM_DECL)->decl_common.decl_flag_3)
> + ?(TREE_CHECK2 (NODE, VAR_DECL, PARM_DECL)->decl_common.decl_flag_2)
> ?#define DECL_VALUE_EXPR(NODE) \
> ? (decl_value_expr_lookup (DECL_WRTL_CHECK (NODE)))
> ?#define SET_DECL_VALUE_EXPR(NODE, VAL) ? ? ? ? ? ? ? ? \
> @@ -2750,11 +2747,11 @@ struct GTY(()) tree_decl_with_rtl {
> ?#define DECL_FCONTEXT(NODE) (FIELD_DECL_CHECK (NODE)->field_decl.fcontext)
>
> ?/* In a FIELD_DECL, indicates this field should be bit-packed. ?*/
> -#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_1)
> +#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->common.base.packed_flag)
>
> ?/* Nonzero in a FIELD_DECL means it is a bit field, and must be accessed
> ? ?specially. ?*/
> -#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_2)
> +#define DECL_BIT_FIELD(NODE) (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_1)
>
> ?/* Used in a FIELD_DECL to indicate that we cannot form the address of
> ? ?this component. ?This makes it possible for Type-Based Alias Analysis
> @@ -2772,7 +2769,7 @@ struct GTY(()) tree_decl_with_rtl {
> ? ?accesses to s.i must not be given the alias set of the type of 'i'
> ? ?(int) but instead directly that of the type of 's' (struct S). ?*/
> ?#define DECL_NONADDRESSABLE_P(NODE) \
> - ?(FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_3)
> + ?(FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_2)
>
> ?struct GTY(()) tree_field_decl {
> ? struct tree_decl_common common;
> @@ -3242,7 +3239,7 @@ struct GTY(()) tree_function_decl {
> ? ?into stabs. ?Instead it will generate cross reference ('x') of names.
> ? ?This uses the same flag as DECL_EXTERNAL. ?*/
> ?#define TYPE_DECL_SUPPRESS_DEBUG(NODE) \
> - ?(TYPE_DECL_CHECK (NODE)->decl_common.decl_flag_2)
> + ?(TYPE_DECL_CHECK (NODE)->decl_common.decl_flag_1)
>
> ?/* Getter of the imported declaration associated to the
> ? ?IMPORTED_DECL node. ?*/
>