> 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
