On Thu, May 31, 2018 at 3:25 PM Eric Botcazou <ebotca...@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 <ebotca...@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 <ebotca...@adacore.com> > > * gcc.dg/store_merging_20.c: New test. > * gnat.dg/opt71.adb: Likewise. > * gnat.dg/opt71_pkg.ads: New helper. > > -- > Eric Botcazou