Re: [RFC] Don't completely scalarize a record if it contains bit-field (PR tree-optimization/45144)

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

On Wed, Aug 4, 2010 at 1:53 PM, Jie Zhang <jie@codesourcery.com> wrote:
> On 08/02/2010 09:01 PM, Martin Jambor wrote:
>>
>> Hi,
>>
>> On Sat, Jul 31, 2010 at 12:09:42AM +0800, Jie Zhang wrote:
>>>
>>> PR tree-optimization/45144 shows an issue that SRA causes. I used
>>> arm-none-eabi target as an example in PR tree-optimization/45144.
>>> But the same issue can also been seen on x86_64-linux-gnu target
>>> using the same test case in the PR.
>>>
>>> SRA completely scalarizes a small record. But when the record is
>>> used later as a whole, GCC has to make the record out of the scalar
>>> parts. When the record contains bit-fields, GCC generates ugly code
>>> to assemble the scalar parts into a record.
>>>
>>> Until the aggregates copy propagation is implemented, I think it
>>> would better to disable full scalarization for such records. The
>>> patch is attached. It's bootstrapped on x86_64-linux-gnu and
>>> regression tested.
>>>
>>> Is it OK for now? We can remove it after aggregates copy propagation
>>> is implemented.
>>>
>>> Will it be better to add bit-field check in
>>> type_consists_of_records_p instead of using a new function
>>> "type_contains_bit_field_p"?
>>>
>>
>> When I was implementing the total scalarization bit of SRA I thought
>> of disabling it for structures with bit-fields too. ?I did not really
>> examine the effects in any way but I never expected this to result in
>> nice code at places where we use SRA to do poor-man's copy
>> propagation. ?However, eventually I decided to keep the total
>> scalarization for these structures because doing so can save stack
>> space and it would be shame if adding one such field to a structure
>> would make us use the space again (in fact, total scalarization was
>> introduced as a fix to unnecessary stack-frame setup bugs like PR
>> 42585). ?But given your results with kernel and gcc, I don't object to
>> disabling it... people will scream if something slows down for them.
>>
>> On the other hand, if we decide to go this way, we need to do the
>> check at a different place, going over the whole type whenever looking
>> at an assignment is not necessary and is wasteful. ?The check would be
>> most appropriate as a part of type_consists_of_records_p where it
>> would be performed only once for each variable in question.
>>
>
> Thanks for your comment! How about this version? I moved the check into
> type_consists_of_records_p. Bootstrapped and regression tested on x86_64.
> Also checked by building linux kernel and made sure there were no
> regressions.

This is ok if Martin is ok with it.

Thanks,
Richard.

>
> Regards,
> --
> Jie Zhang
> CodeSourcery
>