> Just a general remark, the many non-functional but stylistic changes do not > make the patch easier to review ;)
Sorry about that (in this case the ChangeLog is supposed to help a little). > + /* 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? I just copied an existing comment from Jakub a few lines below: /* FIXME: If there is a single chunk of zero bits in mask, perhaps use BIT_INSERT_EXPR instead? */ stmt = gimple_build_assign (make_ssa_name (int_type), BIT_AND_EXPR, tem, mask); so I didn't actually experiment. But since I use BIT_INSERT_EXPR as rhs_code, I wanted to acknowledge the possibility of also generating it. > 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? Alexander provided a valid rebuttal I think. > Otherwise the patch looks OK to me. Thanks for the review. -- Eric Botcazou