Re: [PATCH] Add a new option "-ftree-bitfield-merge" (patch / doc inside)

[Bernhard Reutner-Fischer <rep dot dot dot nop at gmail dot com>]

On Sun, Mar 09, 2014 at 08:35:43PM +0000, Zoran Jovanovic wrote:
> Hello,
> This is new patch version. 
> Approach suggested by Richard Biener with lowering bit-field accesses instead of modifying gimple trees is implemented.
 
> New command line option "-fmerge-bitfields" is introduced.
> 
> Tested - passed gcc regression tests.
> 
> Changelog -
> 
> gcc/ChangeLog:
> 2014-03-09 Zoran Jovanovic (zoran.jovanovic@imgtec.com)
>   * common.opt (fmerge-bitfields): New option.
>   * doc/invoke.texi: Added reference to "-fmerge-bitfields".

Present tense.

>   * tree-sra.c (lower_bitfields): New function.
>   Entry for (-fmerge-bitfields).
>   (bfaccess::hash): New function.
>   (bfaccess::equal): New function.
>   (bfaccess::remove): New function.
>   (bitfield_access_p): New function.
>   (lower_bitfield_read): New function.
>   (lower_bitfield_write): New function.
>   (bitfield_stmt_access_pair_htab_hash): New function.
>   (bitfield_stmt_access_pair_htab_eq): New function.
>   (create_and_insert_access): New function.
>   (get_bit_offset): New function.
>   (get_merged_bit_field_size): New function.
>   (add_stmt_access_pair): New function.
>   (cmp_access): New function.
>   * dwarf2out.c (simple_type_size_in_bits): moved to tree.c.

Present tense. Capital 'M'ove

>   (field_byte_offset): declaration moved to tree.h, static removed.

Capital 'D'eclaration. These are supposed to be sentences. By removing
static you IMHO 'make extern'.

>   * testsuite/gcc.dg/tree-ssa/bitfldmrg1.c: New test.
>   * testsuite/gcc.dg/tree-ssa/bitfldmrg2.c: New test.
>   * tree-ssa-sccvn.c (expressions_equal_p): moved to tree.c.

See above.

>   * tree-ssa-sccvn.h (expressions_equal_p): declaration moved to tree.h.

Likewise.

>   * tree.c (expressions_equal_p): moved from tree-ssa-sccvn.c.

See above.

>   (simple_type_size_in_bits): moved from dwarf2out.c.

See above.

>   * tree.h (expressions_equal_p): declaration added.

Ditto.

>   (field_byte_offset): declaration added.

Ditto.

> 
> Patch -
> 
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 661516d..3331d03 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2193,6 +2193,10 @@ ftree-sra
>  Common Report Var(flag_tree_sra) Optimization
>  Perform scalar replacement of aggregates
>  
> +fmerge-bitfields
> +Common Report Var(flag_tree_bitfield_merge) Init(0) Optimization

Optimization but not enabled for any level. So, where would one
generally want this enabled? CSiBE numbers? SPEC you-name-it
improvements? size(1) improvements where? In GCC there is generally no
interest in the size(1) added to the collection itself, so let me ask
for size(1) and bloat(-o-meter) stats for gcc, cc1 and collect2, just
for the sake of it?

> +Merge loads and stores of consecutive bitfields
> +
>  ftree-ter
>  Common Report Var(flag_tree_ter) Optimization
>  Replace temporary expressions in the SSA->normal pass
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 24bd76e..54bae56 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -411,7 +411,7 @@ Objective-C and Objective-C++ Dialects}.
>  -fsplit-ivs-in-unroller -fsplit-wide-types -fstack-protector @gol
>  -fstack-protector-all -fstack-protector-strong -fstrict-aliasing @gol
>  -fstrict-overflow -fthread-jumps -ftracer -ftree-bit-ccp @gol
> --ftree-builtin-call-dce -ftree-ccp -ftree-ch @gol
> +-fmerge-bitfields -ftree-builtin-call-dce -ftree-ccp -ftree-ch @gol
>  -ftree-coalesce-inline-vars -ftree-coalesce-vars -ftree-copy-prop @gol
>  -ftree-copyrename -ftree-dce -ftree-dominator-opts -ftree-dse @gol
>  -ftree-forwprop -ftree-fre -ftree-loop-if-convert @gol
> @@ -7807,6 +7807,11 @@ pointer alignment information.
>  This pass only operates on local scalar variables and is enabled by default
>  at @option{-O} and higher.  It requires that @option{-ftree-ccp} is enabled.
>  
> +@item -fbitfield-merge

you are talking about '-fmerge-bitfields' up until here (except for
Subject. [Confusion starts here -- Subject: -ftree-bitfield-merge; sofar
Intro -fmerge-bitfields and ChangeLog -fmerge-bitfields]

> +@opindex fmerge-bitfields
> +Combines several adjacent bit-field accesses that copy values
> +from one memory location to another into one single bit-field access.
> +
>  @item -ftree-ccp
>  @opindex ftree-ccp
>  Perform sparse conditional constant propagation (CCP) on trees.  This

> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 284d544..c6a19b2 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -3462,10 +3462,608 @@ perform_intra_sra (void)
>    return ret;
>  }
>  
> +/* Bitfield access and hashtable support commoning same base and
> +   representative.  */
> +
> +struct bfaccess
> +{
> +  bfaccess (tree r):ref (r), r_count (1), w_count (1), merged (false),
> +    modified (false), is_barrier (false), next (0), head_access (0)
> +  {
> +  }
> +
> +  tree ref;
> +  unsigned r_count;  /* Read counter.  */
> +  unsigned w_count;  /* Write counter.  */
> +
> +  /* hash_table support.  */
> +  typedef bfaccess value_type;
> +  typedef bfaccess compare_type;
> +  static inline hashval_t hash (const bfaccess *);

I suspect there's a reason to not have the * be a const* ?

> +  static inline int equal (const bfaccess *, const bfaccess *);

..which holds true here as well.

> +  static inline void remove (bfaccess *);
> +
> +  gimple load_stmt;		/* Bit-field load statement.  */
> +  gimple store_stmt;		/* Bit-field store statement.  */
> +  unsigned src_offset_words;	/* Bit-field offset at src in words.  */
> +  unsigned src_bit_offset;	/* Bit-field offset inside source word.  */
> +  unsigned src_bit_size;	/* Size of bit-field in source word.  */
> +  unsigned dst_offset_words;	/* Bit-field offset at dst in words.  */
> +  unsigned dst_bit_offset;	/* Bit-field offset inside destination
> +				   word.  */
> +  unsigned src_field_offset;	/* Source field offset.  */
> +  unsigned dst_bit_size;	/* Size of bit-field in destination word.  */
> +  tree src_addr;		/* Address of source memory access.  */
> +  tree dst_addr;		/* Address of destination memory access.  */
> +  bool merged;			/* True if access is merged with another
> +				   one.  */
> +  bool modified;		/* True if bit-field size is modified.  */
> +  bool is_barrier;		/* True if access is barrier (call or mem
> +				   access).  */

Back then the above usually were bitfields:1 themselves and for space
considerations were moved to a place where alignment begged for or with
cacheline friendliness in mind but i guess those times are past
nowadays, yes?

> +  struct bfaccess *next;	/* Access with which this one is merged.  */
> +  tree bitfield_representative;	/* Bit field representative of original
> +				   declaration.  */
> +  struct bfaccess *head_access;	/* Head of access list where this one is
> +				   merged.  */
> +};

> +/* Return whether REF is a bitfield access the bit offset of the bitfield

mhm. Maybe it's late here by now, but can you actually parse the
sentence above? Is there an 'at' missing somewhere?
"a bitfield access at" perhaps?

Here I'll stop attempts to follow what you wrote, no offence.

TIA && cheers,