On Wed, May 22, 2013 at 3:57 AM, David Edelsohn <dje....@gmail.com> wrote:
> On Tue, May 21, 2013 at 7:13 PM, Sandra Loosemore
> <san...@codesourcery.com> wrote:
>> On 05/21/2013 04:04 PM, David Edelsohn wrote:
>>>
>>>
>>> There are three issues here:
>>>
>>> 1) Someone in the LTC toolchain team needs to benchmark this patch on
>>> POWER7.
>>
>>
>> That would be great if somebody else could help with that.
>>
>>
>>> 2) We need to clarify how the patch affects the ABI because it cannot
>>> break the ABI.
>>
>>
>> I understand this.
>>
>>
>>> 3) Please stop saying that you cannot justify trying to get the patch
>>> in mainline. Other developers have pointed out how the patch may be
>>> incorrect. Do you want to deliver a broken compiler to CodeSourcery's
>>> customers? The comment sets a bad tone for engaging with the GCC
>>> community.
>>
>>
>> I think you've misunderstood my position, here. Delivering a broken
>> compiler is just what I want to avoid! We've had the original
>> local-arrays-only patch in our local tree for a couple of years now, but we
>> no longer have a customer for it. I thought the comments from the previous
>> review would be straightforward to address and it would be worth making one
>> more attempt to revise and resubmit the patch, but if the feedback we get
>> from the community is that this is still broken in other ways and is going
>> to need a lot more work before it's acceptable, we're going to give up on it
>> and revert the previous version of the patch locally too. We have a lot of
>> higher-priority patches in our local tree that we'd like to get on mainline,
>> and limited resources for working on it, so we need to pick our battles.
>> That's all. :-)
>
> I think the local arrays patch makes sense, if it does not hurt
> performance. We had another recent case where increasing GCC's
> knowledge about the alignment of memory returned by malloc allowed
> additional vectorization opportunities, but hurt performance because
> of bad spilling choices by GCC RA. This alignment patch may expose
> similar RA problems. We may need to apply the patch with the
> optimization disabled until the RA spilling problem is fixed.
>
> Increasing the alignment of arrays within structs and unions would be
> nice, but that probably will change the ABI. I think that they best we
> may be able to do is increase the alignment if the array is the first
> element of the struct or union, see ROUND_TYPE_ALIGN for AIX.
> Although this might be more trouble than it is worth.
Maybe the idea was to increase the alignment of the struct (without
affecting it's layout) when that increases the alignment of a contained
array member. Like for
struct { int i; int j; float a[1024]; int x; };
where aligning to sizeof (int) * 2 would get 'a' a bigger alignment.
Richard.
> Pat or Bill, can you test the performance of the array alignment patch?
>
> Thanks, David
