Re: [patch] Enhance GIMPLE store-merging pass for bit-fields

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

On Thu, May 31, 2018 at 3:25 PM Eric Botcazou <ebotcazou@adacore.com> wrote:
>
> Hi,
>
> this enhances the GIMPLE store-merging pass by teaching it to deal with
> generic stores to bit-fields, i.e. not just stores of immediates.  The
> motivating example is:
>
> struct S {
>   unsigned int flag : 1;
>   unsigned int size : 31;
> };
>
> void foo (struct S *s, unsigned int size)
> {
>   s->flag = 1;
>   s->size = size & 0x7FFFFFFF;
> }
>
> which may seem a little contrived but is the direct translation of something
> very natural in Ada, and for which the compiler currently generates at -O2:
>
>         orb     $1, (%rdi)
>         leal    (%rsi,%rsi), %eax
>         movl    (%rdi), %esi
>         andl    $1, %esi
>         orl     %eax, %esi
>         movl    %esi, (%rdi)
>         ret
>
> With the change, the generated code is optimal:
>
>         leal    1(%rsi,%rsi), %esi
>         movl    %esi, (%rdi)
>         ret
>
> The patch adds a 4th class of stores (with the BIT_INSERT_EXPR code) that can
> be merged into groups containing other BIT_INSERT_EXPR or INTEGER_CST stores.
> These stores are merged like constant stores, but the immediate value is not
> updated (unlike the mask) and instead output_merged_store synthesizes the bit
> insertion sequences from the original stores.  It also contains a few cleanups
> for the dumping code and other minor fixes.
>
> Tested on x86-64/Linux and SPARC/Solaris, OK for the mainline?

Just a general remark, the many non-functional but stylistic changes do not
make the patch easier to review ;)

+         /* If bit insertion is required, we use the source as an accumulator
+            into which the successive bit-field values are manually inserted.
+            FIXME: perhaps use BIT_INSERT_EXPR instead in some cases?  */

so exactly - why not use BIT_INSERT_EXPR here?  Some reasoning would
be nice to have mentioned here.  Is it because BIT_INSERT_EXPR will
basically go the store_bit_field expansion route and thus generate the
same code as the unmerged one?

For code-generating adjacent inserts wouldn't it be more efficient
to do

  accum = first-to-be-inserted-val;
  accum <<= shift-to-next-inserted-val;
  accum |= next-to-be-inserted-val;
...
  accum <<= final-shift;

instead of shifting the to-be-inserted value?

x86 with BMI2 should be able to expand the bit-inserts directly
using pdep (it's a little bit of an awkward instruction though).
BMI doesn't seen to have a single reg-to-bitfield insertion
instruction mimicing bextr.

Otherwise the patch looks OK to me.

Thanks,
Richard.

>
> 2018-05-31  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gimple-ssa-store-merging.c: Include gimple-fold.h.
>         (struct store_immediate_info): Document BIT_INSERT_EXPR stores.
>         (struct merged_store_group): Add bit_insertion field.
>         (dump_char_array): Use standard hexadecimal format.
>         (merged_store_group::merged_store_group): Set bit_insertion to false.
>         (merged_store_group::apply_stores): Use optimal buffer size.  Deal
>         with BIT_INSERT_EXPR stores.  Move up code updating the mask and
>         also print the mask in the dump file.
>         (pass_store_merging::gate): Minor tweak.
>         (imm_store_chain_info::coalesce_immediate): Fix wrong association
>         of stores with groups in dump.  Allow coalescing of BIT_INSERT_EXPR
>         with INTEGER_CST stores.
>         (count_multiple_uses) <BIT_INSERT_EXPR>: New case.
>         (imm_store_chain_info::output_merged_store): Add try_bitpos variable
>         and use it throughout.  Generare bit insertion sequences if need be.
>         (pass_store_merging::process_store): Remove redundant condition.
>         Record store from a SSA name to a bit-field with BIT_INSERT_EXPR.
>
>
> 2018-05-31  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gcc.dg/store_merging_20.c: New test.
>         * gnat.dg/opt71.adb: Likewise.
>         * gnat.dg/opt71_pkg.ads: New helper.
>
> --
> Eric Botcazou