On Sat, Aug 4, 2018 at 12:09 PM, Uros Bizjak <[email protected]> wrote:
> On Sat, Aug 4, 2018 at 3:59 PM, H.J. Lu <[email protected]> wrote:
>> On Sat, Aug 4, 2018 at 3:42 AM, Uros Bizjak <[email protected]> wrote:
>>> On Fri, Aug 3, 2018 at 12:55 AM, H.J. Lu <[email protected]> wrote:
>>>> We should always set cfun->machine->max_used_stack_alignment if the
>>>> maximum stack slot alignment may be greater than 64 bits.
>>>>
>>>> Tested on i686 and x86-64. OK for master and backport for GCC 8?
>>>
>>> Can you explain why 64 bits, and what this value represents? Is this
>>> value the same for 64bit and 32bit targets?
>>>
>>> Should crtl->max_used_stack_slot_alignment be compared to
>>> STACK_BOUNDARY or even MIN_STACK_BOUNDARY instead?
>>
>> In this case, we don't need to realign the incoming stack since both
>> crtl->max_used_stack_slot_alignment and crtl->preferred_stack_boundary
>> are 128 bits. We don't compute the largest alignment of stack slots to
>> keep stack frame properly aligned for it. Normally it is OK. But if
>> the largest alignment of stack slots > 64 bits and we don't keep stack
>> frame proper aligned, we will get segfault if aligned vector load/store
>> are used on these unaligned stack slots. My patch computes the
>> largest alignment of stack slots, which are actually used, if the
>> largest alignment of stack slots allocated is > 64 bits, which is
>> the smallest alignment for misaligned load/store.
>
> Does > 64 bits also hold for 32 bit targets, and -mabi=ms? I think
> that we need to compare to STACK_BOUNDARY instead:
64 bits requirement is independent of any psABIs nor 32-bit vs 64-bit.
cfun->machine->max_used_stack_alignment is used to decide how
stack frame should be aligned. It is always safe to compute it. I used
else if (crtl->max_used_stack_slot_alignment > 64)
to compute cfun->machine->max_used_stack_alignment only if
we have to.
> --cut here--
> Index: config/i386/i386.c
> ===================================================================
> --- config/i386/i386.c (revision 263307)
> +++ config/i386/i386.c (working copy)
> @@ -13281,8 +13281,7 @@
> recompute_frame_layout_p = true;
> }
> }
> - else if (crtl->max_used_stack_slot_alignment
> - > crtl->preferred_stack_boundary)
> + else if (crtl->max_used_stack_slot_alignment > STACK_BOUNDARY)
> {
This isn't correct.. cygming.h has
#define STACK_BOUNDARY (TARGET_64BIT && ix86_abi == MS_ABI ? 128 :
BITS_PER_WORD)
darwin.h has
#undef STACK_BOUNDARY
#define STACK_BOUNDARY \
((profile_flag || (TARGET_64BIT && ix86_abi == MS_ABI)) \
? 128 : BITS_PER_WORD)
i386.h has
/* Boundary (in *bits*) on which stack pointer should be aligned. */
#define STACK_BOUNDARY \
(TARGET_64BIT && ix86_abi == MS_ABI ? 128 : BITS_PER_WORD)
If STACK_BOUNDARY is 128 and max_used_stack_slot_alignment is 128,
we will get segment when 128bit aligned load/store is generated on misaligned
stack slot.
> /* We don't need to realign stack. But we still need to keep
> stack frame properly aligned to satisfy the largest alignment
> --cut here--
>
> (The testcase works OK with -mabi=ms, which somehow suggests that we
> don't need realignment in this case).
We may not hit 128bit aligned load/store on misaligned stack slot in this
case. It doesn't mean that won't happen.
else if (crtl->max_used_stack_slot_alignment > 64)
is the correct thing to do here.
--
H.J.
