Re: PATCH: PR target/46195: r165965 regresses i386 darwin

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Oct 29, 2010 at 10:24 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Fri, Oct 29, 2010 at 7:08 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>>>> This function is a simplified version of contains_aligned_value_p,
>>>> which should return true/false.
>>>
>>> It is not simplified, only the top condition is changed ...
>>>
>>> - ?if (((TARGET_SSE && SSE_REG_MODE_P (mode))
>>> - ? ? ? || mode == TDmode
>>> - ? ? ? || mode == TFmode
>>> - ? ? ? || mode == TCmode)
>>> - ? ? ?&& (!TYPE_USER_ALIGN (type) || TYPE_ALIGN (type) > 128))
>>> - ? ?return true;
>>> + ?if (mode == XFmode || mode == XCmode)
>>> + ? ?return false;
>>>
>>> ... and else was added:
>>>
>>> ?if (AGGREGATE_TYPE_P (type))
>>> ? ?{
>>> ...
>>> ? ?}
>>> + ?else
>>> + ? ?return TYPE_ALIGN (type) >= 128;
>>>
>>> Why can't this be part of existing contains_aligned_value_p?
>>
>> Because contains_aligned_value_p is wrong, which causes:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44948
>> We use contains_aligned_value_p to compute the old/wrong
>> value only when we warn psABI changes.
>
> I see. So, please rename existing contains_aligned_value_p to
> ix86_compat_aligned_value_p, and please add a big comment explaining
> that the function is obsolete and is only be used for compatibility
> with previous versions. For consistency,
> ix86_old_function_arg_boundary should also be renamed to
> ix86_compat_function_arg_boundary and similar comment about obsolete
> compat function should be added there.
>
> New function can then be named ix86_contains_aligned_value_p, since it
> substitutes existing obsolete function.
>
>>>> We have to check mode since type may be NULL. Long double
>>>> is a very special case for 32bit psABI where normal alignment of a
>>>> type/mode != its alignment when passed on stack. ?So we have
>>>> to check when normal alignment is 128 with the alignment of mode/type
>>>> is determined by long double.
>>>
>>> I think that following code is much more readable.
>>>
>>> ?if (!TARGET_64BIT)
>>> ? ?{
>>> ? ? ?/* i386 ABI defines XFmode arguments to be 4 byte aligned. ?*/
>>> ? ? ?if (!type)
>>> ? ? ? ?{
>>> ? ? ? ? ?if (mode == XFmode || mode == XCmode)
>>> ? ? ? ? ? ?align = PARM_BOUNDARY;
>>> ? ? ? ?}
>>> ? ? ?else
>>> ? ? ? ?{
>>> ? ? ? ? ?if (!contains_aligned_value_p (type))
>>> ? ? ? ? ? ?align = PARM_BOUNDARY;
>>
>>
>> contains_aligned_value_p may give the wrong answer
>> and lead to:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44948
>
> Yeah, new function should be used here, but please keep the form above
> for clarity (I hope it is correct, it is untested).
>

Here is the updated patch  Jack, could you please test it on Darwin?
OK for trunk if there are no regressions on Linux and Darwin?

Thanks.

-- 
H.J.
---
2010-10-29  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/46195
	* config/i386/i386.c (contains_aligned_value_p): Renamed to ...
	(ix86_compat_aligned_value_p): This.
	(ix86_old_function_arg_boundary): Updated.
	(ix86_contains_aligned_value_p): New.
	(ix86_function_arg_boundary): Align long double parameters on
	stack to 4byte in 32bit.

Attachment: gcc-pr46195-3.patch
Description: Text document