On Sat, Aug 4, 2018 at 3:59 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, Aug 4, 2018 at 3:42 AM, Uros Bizjak <ubizjak@gmail.com> wrote:>> On Fri, Aug 3, 2018 at 12:55 AM, H.J. Lu <hongjiu.lu@intel.com> 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:
--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)
{
/* 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).
Uros.

On Sun, Aug 5, 2018 at 12:15 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> OK, but please add a comment, so in the future we will still know the>>> purpose of the magic number.>>>>>>> Like this?>>>> H.J.>> --->> cfun->machine->max_used_stack_alignment is used to decide how stack frame>> should be aligned. This is independent of any psABIs nor 32-bit vs 64-bit.>> It is always safe to compute max_used_stack_alignment. We compute it only>> if 128-bit aligned load/store may be generated on misaligned stack slot>> which will lead to segfault.>>>> gcc/>>>> PR target/86386>> * config/i386/i386.c (ix86_finalize_stack_frame_flags): Set>> cfun->machine->max_used_stack_alignment if needed.>>>> gcc/testsuite/>>>> PR target/86386>> * gcc.target/i386/pr86386.c: New file.>> OK, but please write the condition as ">= 128". The number 64 confused> me; I was thinking that it has something to do with minimum alignment> on 64bit target, while 128 clearly shows that alignment is related to> SSE moves. With ">= 128", I think that code is clear enough that a> long comment is not needed.>> Thanks, and sorry for the confusion,> Uros.>
This is what I checked in. I kept the comment change. I will backport it
to GCC 8 after a few days.
Thanks.