This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] i386: Always set cfun->machine->max_used_stack_alignment


On Sat, Aug 4, 2018 at 12:09 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> 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:

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]