| Message ID | 20160127161137.GB16081@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 27, 2016 at 8:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > On 27 Jan 16:44, Jakub Jelinek wrote: >> On Wed, Jan 27, 2016 at 06:34:41PM +0300, Ilya Enkovich wrote: >> > @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >> > opts->x_target_flags |= MASK_VZEROUPPER; >> > if (!(opts_set->x_target_flags & MASK_STV)) >> > opts->x_target_flags |= MASK_STV; >> > + /* Disable STV if -mpreferred-stack-boundary={2,3} - the needed >> >> The comment doesn't match the code, you disable STV only for >> -mpreferred-stack-boundary=2. > > Thanks, here is an updated version. > > Ilya > -- > gcc/ > > 2016-01-27 Jakub Jelinek <jakub@redhat.com> > Ilya Enkovich <enkovich.gnu@gmail.com> > > PR target/69454 > * config/i386/i386.c (convert_scalars_to_vector): Remove > stack alignment fixes. > (ix86_option_override_internal): Disable TARGET_STV if stack > is not properly aligned. > > gcc/testsuite/ > > 2016-01-27 Ilya Enkovich <enkovich.gnu@gmail.com> > > PR target/69454 > * gcc.target/i386/pr69454-1.c: New test. > * gcc.target/i386/pr69454-2.c: New test. > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > index 34b57a4..9fb8db8 100644 > --- a/gcc/config/i386/i386.c > +++ b/gcc/config/i386/i386.c > @@ -3588,16 +3588,6 @@ convert_scalars_to_vector () > bitmap_obstack_release (NULL); > df_process_deferred_rescans (); > > - /* Conversion means we may have 128bit register spills/fills > - which require aligned stack. */ > - if (converted_insns) > - { > - if (crtl->stack_alignment_needed < 128) > - crtl->stack_alignment_needed = 128; > - if (crtl->stack_alignment_estimated < 128) > - crtl->stack_alignment_estimated = 128; > - } > - > return 0; > } > > @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, > opts->x_target_flags |= MASK_VZEROUPPER; > if (!(opts_set->x_target_flags & MASK_STV)) > opts->x_target_flags |= MASK_STV; > + /* Disable STV if -mpreferred-stack-boundary=2 - the needed > + stack realignment will be extra cost the pass doesn't take into > + account and the pass can't realign the stack. */ > + if (ix86_preferred_stack_boundary < 64) > + opts->x_target_flags &= ~MASK_STV; > if (!ix86_tune_features[X86_TUNE_AVX256_UNALIGNED_LOAD_OPTIMAL] > && !(opts_set->x_target_flags & MASK_AVX256_SPLIT_UNALIGNED_LOAD)) > opts->x_target_flags |= MASK_AVX256_SPLIT_UNALIGNED_LOAD; MINIMUM_ALIGNMENT keeps track stack alignment. It is OK to disable STV for -mpreferred-stack-boundary=2. But you should also update ix86_minimum_alignment to make sure that STV is disabled before returning 32 for DImode.
2016-01-27 19:18 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: > On Wed, Jan 27, 2016 at 8:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> On 27 Jan 16:44, Jakub Jelinek wrote: >>> On Wed, Jan 27, 2016 at 06:34:41PM +0300, Ilya Enkovich wrote: >>> > @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>> > opts->x_target_flags |= MASK_VZEROUPPER; >>> > if (!(opts_set->x_target_flags & MASK_STV)) >>> > opts->x_target_flags |= MASK_STV; >>> > + /* Disable STV if -mpreferred-stack-boundary={2,3} - the needed >>> >>> The comment doesn't match the code, you disable STV only for >>> -mpreferred-stack-boundary=2. >> >> Thanks, here is an updated version. >> >> Ilya >> -- >> gcc/ >> >> 2016-01-27 Jakub Jelinek <jakub@redhat.com> >> Ilya Enkovich <enkovich.gnu@gmail.com> >> >> PR target/69454 >> * config/i386/i386.c (convert_scalars_to_vector): Remove >> stack alignment fixes. >> (ix86_option_override_internal): Disable TARGET_STV if stack >> is not properly aligned. >> >> gcc/testsuite/ >> >> 2016-01-27 Ilya Enkovich <enkovich.gnu@gmail.com> >> >> PR target/69454 >> * gcc.target/i386/pr69454-1.c: New test. >> * gcc.target/i386/pr69454-2.c: New test. >> >> >> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >> index 34b57a4..9fb8db8 100644 >> --- a/gcc/config/i386/i386.c >> +++ b/gcc/config/i386/i386.c >> @@ -3588,16 +3588,6 @@ convert_scalars_to_vector () >> bitmap_obstack_release (NULL); >> df_process_deferred_rescans (); >> >> - /* Conversion means we may have 128bit register spills/fills >> - which require aligned stack. */ >> - if (converted_insns) >> - { >> - if (crtl->stack_alignment_needed < 128) >> - crtl->stack_alignment_needed = 128; >> - if (crtl->stack_alignment_estimated < 128) >> - crtl->stack_alignment_estimated = 128; >> - } >> - >> return 0; >> } >> >> @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >> opts->x_target_flags |= MASK_VZEROUPPER; >> if (!(opts_set->x_target_flags & MASK_STV)) >> opts->x_target_flags |= MASK_STV; >> + /* Disable STV if -mpreferred-stack-boundary=2 - the needed >> + stack realignment will be extra cost the pass doesn't take into >> + account and the pass can't realign the stack. */ >> + if (ix86_preferred_stack_boundary < 64) >> + opts->x_target_flags &= ~MASK_STV; >> if (!ix86_tune_features[X86_TUNE_AVX256_UNALIGNED_LOAD_OPTIMAL] >> && !(opts_set->x_target_flags & MASK_AVX256_SPLIT_UNALIGNED_LOAD)) >> opts->x_target_flags |= MASK_AVX256_SPLIT_UNALIGNED_LOAD; > > MINIMUM_ALIGNMENT keeps track stack alignment. It is OK > to disable STV for -mpreferred-stack-boundary=2. But you should > also update ix86_minimum_alignment to make sure that STV is > disabled before returning 32 for DImode. If -mpreferred-stack-boundary=2 then STV is disabled, if STV is enabled then -mpreferred-stack-boundary>=3 and this condition in ix86_minimum_alignment works: if (TARGET_64BIT || align != 64 || ix86_preferred_stack_boundary >= 64) return align; Thanks, Ilya > > > -- > H.J.
On Wed, Jan 27, 2016 at 8:29 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2016-01-27 19:18 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >> On Wed, Jan 27, 2016 at 8:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> On 27 Jan 16:44, Jakub Jelinek wrote: >>>> On Wed, Jan 27, 2016 at 06:34:41PM +0300, Ilya Enkovich wrote: >>>> > @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>>> > opts->x_target_flags |= MASK_VZEROUPPER; >>>> > if (!(opts_set->x_target_flags & MASK_STV)) >>>> > opts->x_target_flags |= MASK_STV; >>>> > + /* Disable STV if -mpreferred-stack-boundary={2,3} - the needed >>>> >>>> The comment doesn't match the code, you disable STV only for >>>> -mpreferred-stack-boundary=2. >>> >>> Thanks, here is an updated version. >>> >>> Ilya >>> -- >>> gcc/ >>> >>> 2016-01-27 Jakub Jelinek <jakub@redhat.com> >>> Ilya Enkovich <enkovich.gnu@gmail.com> >>> >>> PR target/69454 >>> * config/i386/i386.c (convert_scalars_to_vector): Remove >>> stack alignment fixes. >>> (ix86_option_override_internal): Disable TARGET_STV if stack >>> is not properly aligned. >>> >>> gcc/testsuite/ >>> >>> 2016-01-27 Ilya Enkovich <enkovich.gnu@gmail.com> >>> >>> PR target/69454 >>> * gcc.target/i386/pr69454-1.c: New test. >>> * gcc.target/i386/pr69454-2.c: New test. >>> >>> >>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >>> index 34b57a4..9fb8db8 100644 >>> --- a/gcc/config/i386/i386.c >>> +++ b/gcc/config/i386/i386.c >>> @@ -3588,16 +3588,6 @@ convert_scalars_to_vector () >>> bitmap_obstack_release (NULL); >>> df_process_deferred_rescans (); >>> >>> - /* Conversion means we may have 128bit register spills/fills >>> - which require aligned stack. */ >>> - if (converted_insns) >>> - { >>> - if (crtl->stack_alignment_needed < 128) >>> - crtl->stack_alignment_needed = 128; >>> - if (crtl->stack_alignment_estimated < 128) >>> - crtl->stack_alignment_estimated = 128; >>> - } >>> - >>> return 0; >>> } >>> >>> @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>> opts->x_target_flags |= MASK_VZEROUPPER; >>> if (!(opts_set->x_target_flags & MASK_STV)) >>> opts->x_target_flags |= MASK_STV; >>> + /* Disable STV if -mpreferred-stack-boundary=2 - the needed >>> + stack realignment will be extra cost the pass doesn't take into >>> + account and the pass can't realign the stack. */ >>> + if (ix86_preferred_stack_boundary < 64) >>> + opts->x_target_flags &= ~MASK_STV; >>> if (!ix86_tune_features[X86_TUNE_AVX256_UNALIGNED_LOAD_OPTIMAL] >>> && !(opts_set->x_target_flags & MASK_AVX256_SPLIT_UNALIGNED_LOAD)) >>> opts->x_target_flags |= MASK_AVX256_SPLIT_UNALIGNED_LOAD; >> >> MINIMUM_ALIGNMENT keeps track stack alignment. It is OK >> to disable STV for -mpreferred-stack-boundary=2. But you should >> also update ix86_minimum_alignment to make sure that STV is >> disabled before returning 32 for DImode. > > If -mpreferred-stack-boundary=2 then STV is disabled, if STV is enabled then > -mpreferred-stack-boundary>=3 and this condition in > ix86_minimum_alignment works: > > if (TARGET_64BIT || align != 64 || ix86_preferred_stack_boundary >= 64) > return align; > No, you shouldn't make assumptions in ix86_minimum_alignment. You should check explicitly that STV is disabled in ix86_minimum_alignment.
On Wed, Jan 27, 2016 at 8:36 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Wed, Jan 27, 2016 at 8:29 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2016-01-27 19:18 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>> On Wed, Jan 27, 2016 at 8:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>> On 27 Jan 16:44, Jakub Jelinek wrote: >>>>> On Wed, Jan 27, 2016 at 06:34:41PM +0300, Ilya Enkovich wrote: >>>>> > @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>>>> > opts->x_target_flags |= MASK_VZEROUPPER; >>>>> > if (!(opts_set->x_target_flags & MASK_STV)) >>>>> > opts->x_target_flags |= MASK_STV; >>>>> > + /* Disable STV if -mpreferred-stack-boundary={2,3} - the needed >>>>> >>>>> The comment doesn't match the code, you disable STV only for >>>>> -mpreferred-stack-boundary=2. >>>> >>>> Thanks, here is an updated version. >>>> >>>> Ilya >>>> -- >>>> gcc/ >>>> >>>> 2016-01-27 Jakub Jelinek <jakub@redhat.com> >>>> Ilya Enkovich <enkovich.gnu@gmail.com> >>>> >>>> PR target/69454 >>>> * config/i386/i386.c (convert_scalars_to_vector): Remove >>>> stack alignment fixes. >>>> (ix86_option_override_internal): Disable TARGET_STV if stack >>>> is not properly aligned. >>>> >>>> gcc/testsuite/ >>>> >>>> 2016-01-27 Ilya Enkovich <enkovich.gnu@gmail.com> >>>> >>>> PR target/69454 >>>> * gcc.target/i386/pr69454-1.c: New test. >>>> * gcc.target/i386/pr69454-2.c: New test. >>>> >>>> >>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >>>> index 34b57a4..9fb8db8 100644 >>>> --- a/gcc/config/i386/i386.c >>>> +++ b/gcc/config/i386/i386.c >>>> @@ -3588,16 +3588,6 @@ convert_scalars_to_vector () >>>> bitmap_obstack_release (NULL); >>>> df_process_deferred_rescans (); >>>> >>>> - /* Conversion means we may have 128bit register spills/fills >>>> - which require aligned stack. */ >>>> - if (converted_insns) >>>> - { >>>> - if (crtl->stack_alignment_needed < 128) >>>> - crtl->stack_alignment_needed = 128; >>>> - if (crtl->stack_alignment_estimated < 128) >>>> - crtl->stack_alignment_estimated = 128; >>>> - } >>>> - >>>> return 0; >>>> } >>>> >>>> @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>>> opts->x_target_flags |= MASK_VZEROUPPER; >>>> if (!(opts_set->x_target_flags & MASK_STV)) >>>> opts->x_target_flags |= MASK_STV; >>>> + /* Disable STV if -mpreferred-stack-boundary=2 - the needed >>>> + stack realignment will be extra cost the pass doesn't take into >>>> + account and the pass can't realign the stack. */ >>>> + if (ix86_preferred_stack_boundary < 64) >>>> + opts->x_target_flags &= ~MASK_STV; This won't work for 32-bit incoming stack boundary and 64-bit preferred stack boundary. In this case, STV won't be off. When LRA needs 64-bit aligned stack slot, stack must be realigned. But for leaf function, we may not realign stack if ix86_minimum_alignment returns 32 for DImode. You must either add assert (!TARGET_STV) before returning 32 for DImode or return 64 for DImode if STV is on in ix86_minimum_alignment. >>>> if (!ix86_tune_features[X86_TUNE_AVX256_UNALIGNED_LOAD_OPTIMAL] >>>> && !(opts_set->x_target_flags & MASK_AVX256_SPLIT_UNALIGNED_LOAD)) >>>> opts->x_target_flags |= MASK_AVX256_SPLIT_UNALIGNED_LOAD; >>> >>> MINIMUM_ALIGNMENT keeps track stack alignment. It is OK >>> to disable STV for -mpreferred-stack-boundary=2. But you should >>> also update ix86_minimum_alignment to make sure that STV is >>> disabled before returning 32 for DImode. >> >> If -mpreferred-stack-boundary=2 then STV is disabled, if STV is enabled then >> -mpreferred-stack-boundary>=3 and this condition in >> ix86_minimum_alignment works: >> >> if (TARGET_64BIT || align != 64 || ix86_preferred_stack_boundary >= 64) >> return align; >> > > No, you shouldn't make assumptions in ix86_minimum_alignment. You > should check explicitly that STV is disabled in ix86_minimum_alignment. > > > -- > H.J.
2016-01-28 9:00 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: > On Wed, Jan 27, 2016 at 8:36 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Wed, Jan 27, 2016 at 8:29 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> 2016-01-27 19:18 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>> On Wed, Jan 27, 2016 at 8:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>>> On 27 Jan 16:44, Jakub Jelinek wrote: >>>>>> On Wed, Jan 27, 2016 at 06:34:41PM +0300, Ilya Enkovich wrote: >>>>>> > @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>>>>> > opts->x_target_flags |= MASK_VZEROUPPER; >>>>>> > if (!(opts_set->x_target_flags & MASK_STV)) >>>>>> > opts->x_target_flags |= MASK_STV; >>>>>> > + /* Disable STV if -mpreferred-stack-boundary={2,3} - the needed >>>>>> >>>>>> The comment doesn't match the code, you disable STV only for >>>>>> -mpreferred-stack-boundary=2. >>>>> >>>>> Thanks, here is an updated version. >>>>> >>>>> Ilya >>>>> -- >>>>> gcc/ >>>>> >>>>> 2016-01-27 Jakub Jelinek <jakub@redhat.com> >>>>> Ilya Enkovich <enkovich.gnu@gmail.com> >>>>> >>>>> PR target/69454 >>>>> * config/i386/i386.c (convert_scalars_to_vector): Remove >>>>> stack alignment fixes. >>>>> (ix86_option_override_internal): Disable TARGET_STV if stack >>>>> is not properly aligned. >>>>> >>>>> gcc/testsuite/ >>>>> >>>>> 2016-01-27 Ilya Enkovich <enkovich.gnu@gmail.com> >>>>> >>>>> PR target/69454 >>>>> * gcc.target/i386/pr69454-1.c: New test. >>>>> * gcc.target/i386/pr69454-2.c: New test. >>>>> >>>>> >>>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >>>>> index 34b57a4..9fb8db8 100644 >>>>> --- a/gcc/config/i386/i386.c >>>>> +++ b/gcc/config/i386/i386.c >>>>> @@ -3588,16 +3588,6 @@ convert_scalars_to_vector () >>>>> bitmap_obstack_release (NULL); >>>>> df_process_deferred_rescans (); >>>>> >>>>> - /* Conversion means we may have 128bit register spills/fills >>>>> - which require aligned stack. */ >>>>> - if (converted_insns) >>>>> - { >>>>> - if (crtl->stack_alignment_needed < 128) >>>>> - crtl->stack_alignment_needed = 128; >>>>> - if (crtl->stack_alignment_estimated < 128) >>>>> - crtl->stack_alignment_estimated = 128; >>>>> - } >>>>> - >>>>> return 0; >>>>> } >>>>> >>>>> @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>>>> opts->x_target_flags |= MASK_VZEROUPPER; >>>>> if (!(opts_set->x_target_flags & MASK_STV)) >>>>> opts->x_target_flags |= MASK_STV; >>>>> + /* Disable STV if -mpreferred-stack-boundary=2 - the needed >>>>> + stack realignment will be extra cost the pass doesn't take into >>>>> + account and the pass can't realign the stack. */ >>>>> + if (ix86_preferred_stack_boundary < 64) >>>>> + opts->x_target_flags &= ~MASK_STV; > > This won't work for 32-bit incoming stack boundary and 64-bit preferred > stack boundary. In this case, STV won't be off. When LRA needs 64-bit > aligned stack slot, stack must be realigned. But for leaf function, we may > not realign stack if ix86_minimum_alignment returns 32 for DImode. You > must either add assert (!TARGET_STV) before returning 32 for DImode or > return 64 for DImode if STV is on in ix86_minimum_alignment. TARGET_STV doesn't mean STV pass will run. We can check alignment in STV pass gate and this assert would be wrong. If we decide STV to be dependent on stack alignment then we shouldn't make alignment be dependent on STV. I can add assert into convert_scalars_to_vector to check crtl->stack_alignment_estimated >= 64 by that moment. Thanks, Ilya > >>>>> if (!ix86_tune_features[X86_TUNE_AVX256_UNALIGNED_LOAD_OPTIMAL] >>>>> && !(opts_set->x_target_flags & MASK_AVX256_SPLIT_UNALIGNED_LOAD)) >>>>> opts->x_target_flags |= MASK_AVX256_SPLIT_UNALIGNED_LOAD; >>>> >>>> MINIMUM_ALIGNMENT keeps track stack alignment. It is OK >>>> to disable STV for -mpreferred-stack-boundary=2. But you should >>>> also update ix86_minimum_alignment to make sure that STV is >>>> disabled before returning 32 for DImode. >>> >>> If -mpreferred-stack-boundary=2 then STV is disabled, if STV is enabled then >>> -mpreferred-stack-boundary>=3 and this condition in >>> ix86_minimum_alignment works: >>> >>> if (TARGET_64BIT || align != 64 || ix86_preferred_stack_boundary >= 64) >>> return align; >>> >> >> No, you shouldn't make assumptions in ix86_minimum_alignment. You >> should check explicitly that STV is disabled in ix86_minimum_alignment. >> >> >> -- >> H.J. > > > > -- > H.J.
On Thu, Jan 28, 2016 at 2:06 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2016-01-28 9:00 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >> On Wed, Jan 27, 2016 at 8:36 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Wed, Jan 27, 2016 at 8:29 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>> 2016-01-27 19:18 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>> On Wed, Jan 27, 2016 at 8:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>>>> On 27 Jan 16:44, Jakub Jelinek wrote: >>>>>>> On Wed, Jan 27, 2016 at 06:34:41PM +0300, Ilya Enkovich wrote: >>>>>>> > @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>>>>>> > opts->x_target_flags |= MASK_VZEROUPPER; >>>>>>> > if (!(opts_set->x_target_flags & MASK_STV)) >>>>>>> > opts->x_target_flags |= MASK_STV; >>>>>>> > + /* Disable STV if -mpreferred-stack-boundary={2,3} - the needed >>>>>>> >>>>>>> The comment doesn't match the code, you disable STV only for >>>>>>> -mpreferred-stack-boundary=2. >>>>>> >>>>>> Thanks, here is an updated version. >>>>>> >>>>>> Ilya >>>>>> -- >>>>>> gcc/ >>>>>> >>>>>> 2016-01-27 Jakub Jelinek <jakub@redhat.com> >>>>>> Ilya Enkovich <enkovich.gnu@gmail.com> >>>>>> >>>>>> PR target/69454 >>>>>> * config/i386/i386.c (convert_scalars_to_vector): Remove >>>>>> stack alignment fixes. >>>>>> (ix86_option_override_internal): Disable TARGET_STV if stack >>>>>> is not properly aligned. >>>>>> >>>>>> gcc/testsuite/ >>>>>> >>>>>> 2016-01-27 Ilya Enkovich <enkovich.gnu@gmail.com> >>>>>> >>>>>> PR target/69454 >>>>>> * gcc.target/i386/pr69454-1.c: New test. >>>>>> * gcc.target/i386/pr69454-2.c: New test. >>>>>> >>>>>> >>>>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c >>>>>> index 34b57a4..9fb8db8 100644 >>>>>> --- a/gcc/config/i386/i386.c >>>>>> +++ b/gcc/config/i386/i386.c >>>>>> @@ -3588,16 +3588,6 @@ convert_scalars_to_vector () >>>>>> bitmap_obstack_release (NULL); >>>>>> df_process_deferred_rescans (); >>>>>> >>>>>> - /* Conversion means we may have 128bit register spills/fills >>>>>> - which require aligned stack. */ >>>>>> - if (converted_insns) >>>>>> - { >>>>>> - if (crtl->stack_alignment_needed < 128) >>>>>> - crtl->stack_alignment_needed = 128; >>>>>> - if (crtl->stack_alignment_estimated < 128) >>>>>> - crtl->stack_alignment_estimated = 128; >>>>>> - } >>>>>> - >>>>>> return 0; >>>>>> } >>>>>> >>>>>> @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, >>>>>> opts->x_target_flags |= MASK_VZEROUPPER; >>>>>> if (!(opts_set->x_target_flags & MASK_STV)) >>>>>> opts->x_target_flags |= MASK_STV; >>>>>> + /* Disable STV if -mpreferred-stack-boundary=2 - the needed >>>>>> + stack realignment will be extra cost the pass doesn't take into >>>>>> + account and the pass can't realign the stack. */ >>>>>> + if (ix86_preferred_stack_boundary < 64) >>>>>> + opts->x_target_flags &= ~MASK_STV; >> >> This won't work for 32-bit incoming stack boundary and 64-bit preferred >> stack boundary. In this case, STV won't be off. When LRA needs 64-bit >> aligned stack slot, stack must be realigned. But for leaf function, we may >> not realign stack if ix86_minimum_alignment returns 32 for DImode. You >> must either add assert (!TARGET_STV) before returning 32 for DImode or >> return 64 for DImode if STV is on in ix86_minimum_alignment. > > TARGET_STV doesn't mean STV pass will run. We can check alignment in STV > pass gate and this assert would be wrong. If we decide STV to be dependent on > stack alignment then we shouldn't make alignment be dependent on STV. I can add > assert into convert_scalars_to_vector to check > crtl->stack_alignment_estimated >= 64 > by that moment. > The bottom line is ix86_minimum_alignment must return the correct number for DImode or you can just turn off STV. My suggestion is to use my patch.
On Thu, Jan 28, 2016 at 04:42:02AM -0800, H.J. Lu wrote: > >>>>>> 2016-01-27 Jakub Jelinek <jakub@redhat.com> > >>>>>> Ilya Enkovich <enkovich.gnu@gmail.com> > >>>>>> > >>>>>> PR target/69454 > >>>>>> * config/i386/i386.c (convert_scalars_to_vector): Remove > >>>>>> stack alignment fixes. > >>>>>> (ix86_option_override_internal): Disable TARGET_STV if stack > >>>>>> is not properly aligned. > >>>>>> > >>>>>> gcc/testsuite/ > >>>>>> > >>>>>> 2016-01-27 Ilya Enkovich <enkovich.gnu@gmail.com> > >>>>>> > >>>>>> PR target/69454 > >>>>>> * gcc.target/i386/pr69454-1.c: New test. > >>>>>> * gcc.target/i386/pr69454-2.c: New test. > >>>>>> > >>>>>> > >>>>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > >>>>>> index 34b57a4..9fb8db8 100644 > >>>>>> --- a/gcc/config/i386/i386.c > >>>>>> +++ b/gcc/config/i386/i386.c > >>>>>> @@ -3588,16 +3588,6 @@ convert_scalars_to_vector () > >>>>>> bitmap_obstack_release (NULL); > >>>>>> df_process_deferred_rescans (); > >>>>>> > >>>>>> - /* Conversion means we may have 128bit register spills/fills > >>>>>> - which require aligned stack. */ > >>>>>> - if (converted_insns) > >>>>>> - { > >>>>>> - if (crtl->stack_alignment_needed < 128) > >>>>>> - crtl->stack_alignment_needed = 128; > >>>>>> - if (crtl->stack_alignment_estimated < 128) > >>>>>> - crtl->stack_alignment_estimated = 128; > >>>>>> - } > >>>>>> - > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, > >>>>>> opts->x_target_flags |= MASK_VZEROUPPER; > >>>>>> if (!(opts_set->x_target_flags & MASK_STV)) > >>>>>> opts->x_target_flags |= MASK_STV; > >>>>>> + /* Disable STV if -mpreferred-stack-boundary=2 - the needed > >>>>>> + stack realignment will be extra cost the pass doesn't take into > >>>>>> + account and the pass can't realign the stack. */ > >>>>>> + if (ix86_preferred_stack_boundary < 64) > >>>>>> + opts->x_target_flags &= ~MASK_STV; > >> > >> This won't work for 32-bit incoming stack boundary and 64-bit preferred > >> stack boundary. In this case, STV won't be off. When LRA needs 64-bit > >> aligned stack slot, stack must be realigned. But for leaf function, we may > >> not realign stack if ix86_minimum_alignment returns 32 for DImode. You > >> must either add assert (!TARGET_STV) before returning 32 for DImode or > >> return 64 for DImode if STV is on in ix86_minimum_alignment. > > > > TARGET_STV doesn't mean STV pass will run. We can check alignment in STV > > pass gate and this assert would be wrong. If we decide STV to be dependent on > > stack alignment then we shouldn't make alignment be dependent on STV. I can add > > assert into convert_scalars_to_vector to check > > crtl->stack_alignment_estimated >= 64 > > by that moment. > > > > The bottom line is ix86_minimum_alignment must return the correct > number for DImode or you can just turn off STV. My suggestion is > to use my patch. Uros, any preferences here? I mean, it is possible to use e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment change as a safety net, in the usual case for -mpreferred-stack-boundary=2 we'll just disable TARGET_STV and ix86_minimum_alignment change won't do anything, as TARGET_STV will be false, and if for whatever case it gets through (target attribute, -mincoming-stack-boundary=, ...) ix86_minimum_alignment will be there to ensure enough stack alignment. Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, and that is something we don't want to affect. Jakub
On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> The bottom line is ix86_minimum_alignment must return the correct >> number for DImode or you can just turn off STV. My suggestion is >> to use my patch. > > Uros, any preferences here? I mean, it is possible to use > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do > anything, as TARGET_STV will be false, and if for whatever case it gets > through (target attribute, -mincoming-stack-boundary=, ...) > ix86_minimum_alignment will be there to ensure enough stack alignment. > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, > and that is something we don't want to affect. IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV is only an optimization. Perhaps we can also emit a "sorry" for explicit -mstv in case stack boundary requirement is not satisfied. *If* there is a need for -mstv with smaller stack boundary, we can revisit this decision for later gcc versions. I think disabling STV is less surprising option than increasing stack boundary behind the user's back. Uros.
On Tue, Feb 2, 2016 at 4:24 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: > >>> The bottom line is ix86_minimum_alignment must return the correct >>> number for DImode or you can just turn off STV. My suggestion is >>> to use my patch. >> >> Uros, any preferences here? I mean, it is possible to use >> e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >> change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >> we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >> anything, as TARGET_STV will be false, and if for whatever case it gets >> through (target attribute, -mincoming-stack-boundary=, ...) >> ix86_minimum_alignment will be there to ensure enough stack alignment. >> Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >> and that is something we don't want to affect. > > IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV > is only an optimization. Perhaps we can also emit a "sorry" for > explicit -mstv in case stack boundary requirement is not satisfied. > *If* there is a need for -mstv with smaller stack boundary, we can > revisit this decision for later gcc versions. > > I think disabling STV is less surprising option than increasing stack > boundary behind the user's back. > > Uros. My ix86_minimum_alignment change is for correctness, which is required for DImode spill/fill unless LRA supports unaligned spill/fill. Disabling TARGET_STV for -mpreferred-stack-boundary=2 is an optimization. Let me point it out again. Checking -mpreferred-stack-boundary < 3 to disable STV won't work for 32-bit incoming stack boundary and 64-bit preferred stack boundary. In this case, STV is on. When LRA needs 64-bit aligned stack slot, stack must be realigned. But for leaf function, we may not realign stack if ix86_minimum_alignment returns 32 for DImode. We must return 64 for DImode if STV is on in ix86_minimum_alignment.
On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: > On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > >> The bottom line is ix86_minimum_alignment must return the correct > >> number for DImode or you can just turn off STV. My suggestion is > >> to use my patch. > > > > Uros, any preferences here? I mean, it is possible to use > > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment > > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 > > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do > > anything, as TARGET_STV will be false, and if for whatever case it gets > > through (target attribute, -mincoming-stack-boundary=, ...) > > ix86_minimum_alignment will be there to ensure enough stack alignment. > > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, > > and that is something we don't want to affect. > > IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV > is only an optimization. Perhaps we can also emit a "sorry" for > explicit -mstv in case stack boundary requirement is not satisfied. > *If* there is a need for -mstv with smaller stack boundary, we can > revisit this decision for later gcc versions. > > I think disabling STV is less surprising option than increasing stack > boundary behind the user's back. So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html ok for trunk then (alone or with additional sorry, incremental or not?)? I believe it does just that. Jakub
On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> >> >> The bottom line is ix86_minimum_alignment must return the correct >> >> number for DImode or you can just turn off STV. My suggestion is >> >> to use my patch. >> > >> > Uros, any preferences here? I mean, it is possible to use >> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >> > anything, as TARGET_STV will be false, and if for whatever case it gets >> > through (target attribute, -mincoming-stack-boundary=, ...) >> > ix86_minimum_alignment will be there to ensure enough stack alignment. >> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >> > and that is something we don't want to affect. >> >> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >> is only an optimization. Perhaps we can also emit a "sorry" for >> explicit -mstv in case stack boundary requirement is not satisfied. >> *If* there is a need for -mstv with smaller stack boundary, we can >> revisit this decision for later gcc versions. >> >> I think disabling STV is less surprising option than increasing stack >> boundary behind the user's back. > > So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html > ok for trunk then (alone or with additional sorry, incremental or not?)? > I believe it does just that. This patch is WRONG.
On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>> >>> >> The bottom line is ix86_minimum_alignment must return the correct >>> >> number for DImode or you can just turn off STV. My suggestion is >>> >> to use my patch. >>> > >>> > Uros, any preferences here? I mean, it is possible to use >>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>> > through (target attribute, -mincoming-stack-boundary=, ...) >>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>> > and that is something we don't want to affect. >>> >>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>> is only an optimization. Perhaps we can also emit a "sorry" for >>> explicit -mstv in case stack boundary requirement is not satisfied. >>> *If* there is a need for -mstv with smaller stack boundary, we can >>> revisit this decision for later gcc versions. >>> >>> I think disabling STV is less surprising option than increasing stack >>> boundary behind the user's back. >> >> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >> ok for trunk then (alone or with additional sorry, incremental or not?)? >> I believe it does just that. > > This patch is WRONG. > > -- > H.J. You will run into the same ICE with -mincoming-stack-boundary=2 -msse2 -O2 -m32 in a leaf function which needs DImode spill/fill.
2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: > On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>> >>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>> >> number for DImode or you can just turn off STV. My suggestion is >>>> >> to use my patch. >>>> > >>>> > Uros, any preferences here? I mean, it is possible to use >>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>> > and that is something we don't want to affect. >>>> >>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>> revisit this decision for later gcc versions. >>>> >>>> I think disabling STV is less surprising option than increasing stack >>>> boundary behind the user's back. >>> >>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>> I believe it does just that. >> >> This patch is WRONG. >> >> -- >> H.J. > > You will run into the same ICE with > > -mincoming-stack-boundary=2 -msse2 -O2 -m32 > > in a leaf function which needs DImode spill/fill. Why would we need DImode spill/fill having no DImode registers? Thanks, Ilya > > > -- > H.J.
On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>> >>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>> >> to use my patch. >>>>> > >>>>> > Uros, any preferences here? I mean, it is possible to use >>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>> > and that is something we don't want to affect. >>>>> >>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>> revisit this decision for later gcc versions. >>>>> >>>>> I think disabling STV is less surprising option than increasing stack >>>>> boundary behind the user's back. >>>> >>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>> I believe it does just that. >>> >>> This patch is WRONG. >>> >>> -- >>> H.J. >> >> You will run into the same ICE with >> >> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >> >> in a leaf function which needs DImode spill/fill. > > Why would we need DImode spill/fill having no DImode registers? > Because STV is enabled with -mincoming-stack-boundary=2 -msse2 -O2 -m32
On Tue, Feb 2, 2016 at 2:06 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>> >>>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>>> >> to use my patch. >>>>>> > >>>>>> > Uros, any preferences here? I mean, it is possible to use >>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>>> > and that is something we don't want to affect. >>>>>> >>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>>> revisit this decision for later gcc versions. >>>>>> >>>>>> I think disabling STV is less surprising option than increasing stack >>>>>> boundary behind the user's back. >>>>> >>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>>> I believe it does just that. >>>> >>>> This patch is WRONG. >>>> >>>> -- >>>> H.J. >>> >>> You will run into the same ICE with >>> >>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>> >>> in a leaf function which needs DImode spill/fill. >> >> Why would we need DImode spill/fill having no DImode registers? >> > > Because STV is enabled with > > -mincoming-stack-boundary=2 -msse2 -O2 -m32 But this is the whole trick. Since stack alignment requirements won't be satisfied, we disable STV even with -msse2. Uros.
On Tue, Feb 02, 2016 at 04:46:26AM -0800, H.J. Lu wrote: > >> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html > >> ok for trunk then (alone or with additional sorry, incremental or not?)? > >> I believe it does just that. > > > > This patch is WRONG. > > > > -- > > H.J. > > You will run into the same ICE with > > -mincoming-stack-boundary=2 -msse2 -O2 -m32 > > in a leaf function which needs DImode spill/fill. So are you arguing for changing + /* Disable STV if -mpreferred-stack-boundary=2 - the needed + stack realignment will be extra cost the pass doesn't take into + account and the pass can't realign the stack. */ + if (ix86_preferred_stack_boundary < 64) + opts->x_target_flags &= ~MASK_STV; to + /* Disable STV if -m{preferred,incoming}-stack-boundary=2 - the needed + stack realignment will be extra cost the pass doesn't take into + account and the pass can't realign the stack. */ + if (ix86_preferred_stack_boundary < 64 + || ix86_incoming_stack_boundary < 64) + opts->x_target_flags &= ~MASK_STV; I'm fine with that. Jakub
2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: > On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>> >>>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>>> >> to use my patch. >>>>>> > >>>>>> > Uros, any preferences here? I mean, it is possible to use >>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>>> > and that is something we don't want to affect. >>>>>> >>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>>> revisit this decision for later gcc versions. >>>>>> >>>>>> I think disabling STV is less surprising option than increasing stack >>>>>> boundary behind the user's back. >>>>> >>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>>> I believe it does just that. >>>> >>>> This patch is WRONG. >>>> >>>> -- >>>> H.J. >>> >>> You will run into the same ICE with >>> >>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>> >>> in a leaf function which needs DImode spill/fill. >> >> Why would we need DImode spill/fill having no DImode registers? >> > > Because STV is enabled with > > -mincoming-stack-boundary=2 -msse2 -O2 -m32 I misread it as -mpreferred-... So why would we fail having a proper preferred stack alignment? AFAIK leaf function doesn't affect alignment until we finalize it after RA. > > > -- > H.J.
On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>> >>>>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>>>> >> to use my patch. >>>>>>> > >>>>>>> > Uros, any preferences here? I mean, it is possible to use >>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>>>> > and that is something we don't want to affect. >>>>>>> >>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>>>> revisit this decision for later gcc versions. >>>>>>> >>>>>>> I think disabling STV is less surprising option than increasing stack >>>>>>> boundary behind the user's back. >>>>>> >>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>>>> I believe it does just that. >>>>> >>>>> This patch is WRONG. >>>>> >>>>> -- >>>>> H.J. >>>> >>>> You will run into the same ICE with >>>> >>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>> >>>> in a leaf function which needs DImode spill/fill. >>> >>> Why would we need DImode spill/fill having no DImode registers? >>> >> >> Because STV is enabled with >> >> -mincoming-stack-boundary=2 -msse2 -O2 -m32 > > I misread it as -mpreferred-... So why would we fail having a proper > preferred stack alignment? AFAIK leaf function doesn't affect > alignment until we finalize it after RA. > /* Finalize stack_realign_needed flag, which will guide prologue/epilogue to be generated in correct form. */ static void ix86_finalize_stack_realign_flags (void) { /* Check if stack realign is really needed after reload, and stores result in cfun */ unsigned int incoming_stack_boundary = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary); unsigned int stack_realign = (incoming_stack_boundary < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor ? crtl->max_used_stack_slot_alignment ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ For leaf function, we check max_used_stack_slot_alignment. Since ix86_minimum_alignment returns 32 for DImode. We won't realign stack for DImode spill/fill. : crtl->stack_alignment_needed));
On Tue, Feb 2, 2016 at 5:09 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Feb 02, 2016 at 04:46:26AM -0800, H.J. Lu wrote: >> >> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >> >> ok for trunk then (alone or with additional sorry, incremental or not?)? >> >> I believe it does just that. >> > >> > This patch is WRONG. >> > >> > -- >> > H.J. >> >> You will run into the same ICE with >> >> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >> >> in a leaf function which needs DImode spill/fill. > > So are you arguing for changing > + /* Disable STV if -mpreferred-stack-boundary=2 - the needed > + stack realignment will be extra cost the pass doesn't take into > + account and the pass can't realign the stack. */ > + if (ix86_preferred_stack_boundary < 64) > + opts->x_target_flags &= ~MASK_STV; > to > + /* Disable STV if -m{preferred,incoming}-stack-boundary=2 - the needed > + stack realignment will be extra cost the pass doesn't take into > + account and the pass can't realign the stack. */ > + if (ix86_preferred_stack_boundary < 64 > + || ix86_incoming_stack_boundary < 64) > + opts->x_target_flags &= ~MASK_STV; > I'm fine with that. > There are many checks/asserts for stack alignment. At minimum, we should assert STV is off in ix86_minimum_alignment before returning 32 for DImode.
2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: > On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>> >>>>>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>>>>> >> to use my patch. >>>>>>>> > >>>>>>>> > Uros, any preferences here? I mean, it is possible to use >>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>>>>> > and that is something we don't want to affect. >>>>>>>> >>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>>>>> revisit this decision for later gcc versions. >>>>>>>> >>>>>>>> I think disabling STV is less surprising option than increasing stack >>>>>>>> boundary behind the user's back. >>>>>>> >>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>>>>> I believe it does just that. >>>>>> >>>>>> This patch is WRONG. >>>>>> >>>>>> -- >>>>>> H.J. >>>>> >>>>> You will run into the same ICE with >>>>> >>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>>> >>>>> in a leaf function which needs DImode spill/fill. >>>> >>>> Why would we need DImode spill/fill having no DImode registers? >>>> >>> >>> Because STV is enabled with >>> >>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >> >> I misread it as -mpreferred-... So why would we fail having a proper >> preferred stack alignment? AFAIK leaf function doesn't affect >> alignment until we finalize it after RA. >> > > /* Finalize stack_realign_needed flag, which will guide prologue/epilogue > to be generated in correct form. */ > static void > ix86_finalize_stack_realign_flags (void) > { > /* Check if stack realign is really needed after reload, and > stores result in cfun */ > unsigned int incoming_stack_boundary > = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary > ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary); > unsigned int stack_realign > = (incoming_stack_boundary > < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor > ? crtl->max_used_stack_slot_alignment > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ We call it after RA when all spill slots are allocated and check if we may relax stack alignment. Don't see any problem here. Thanks, Ilya > > For leaf function, we check max_used_stack_slot_alignment. > Since ix86_minimum_alignment returns 32 for DImode. > We won't realign stack for DImode spill/fill. > > : crtl->stack_alignment_needed)); > > > -- > H.J.
On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>>> >>>>>>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>>>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>>>>>> >> to use my patch. >>>>>>>>> > >>>>>>>>> > Uros, any preferences here? I mean, it is possible to use >>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>>>>>> > and that is something we don't want to affect. >>>>>>>>> >>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>>>>>> revisit this decision for later gcc versions. >>>>>>>>> >>>>>>>>> I think disabling STV is less surprising option than increasing stack >>>>>>>>> boundary behind the user's back. >>>>>>>> >>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>>>>>> I believe it does just that. >>>>>>> >>>>>>> This patch is WRONG. >>>>>>> >>>>>>> -- >>>>>>> H.J. >>>>>> >>>>>> You will run into the same ICE with >>>>>> >>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>>>> >>>>>> in a leaf function which needs DImode spill/fill. >>>>> >>>>> Why would we need DImode spill/fill having no DImode registers? >>>>> >>>> >>>> Because STV is enabled with >>>> >>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>> >>> I misread it as -mpreferred-... So why would we fail having a proper >>> preferred stack alignment? AFAIK leaf function doesn't affect >>> alignment until we finalize it after RA. >>> >> >> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue >> to be generated in correct form. */ >> static void >> ix86_finalize_stack_realign_flags (void) >> { >> /* Check if stack realign is really needed after reload, and >> stores result in cfun */ >> unsigned int incoming_stack_boundary >> = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary >> ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary); >> unsigned int stack_realign >> = (incoming_stack_boundary >> < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor >> ? crtl->max_used_stack_slot_alignment >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > We call it after RA when all spill slots are allocated and check if we > may relax stack alignment. Don't see any problem here. Please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26 Why did LRA crash then?
2016-02-02 16:25 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: > On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>>>> >>>>>>>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>>>>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>>>>>>> >> to use my patch. >>>>>>>>>> > >>>>>>>>>> > Uros, any preferences here? I mean, it is possible to use >>>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>>>>>>> > and that is something we don't want to affect. >>>>>>>>>> >>>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>>>>>>> revisit this decision for later gcc versions. >>>>>>>>>> >>>>>>>>>> I think disabling STV is less surprising option than increasing stack >>>>>>>>>> boundary behind the user's back. >>>>>>>>> >>>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>>>>>>> I believe it does just that. >>>>>>>> >>>>>>>> This patch is WRONG. >>>>>>>> >>>>>>>> -- >>>>>>>> H.J. >>>>>>> >>>>>>> You will run into the same ICE with >>>>>>> >>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>>>>> >>>>>>> in a leaf function which needs DImode spill/fill. >>>>>> >>>>>> Why would we need DImode spill/fill having no DImode registers? >>>>>> >>>>> >>>>> Because STV is enabled with >>>>> >>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>> >>>> I misread it as -mpreferred-... So why would we fail having a proper >>>> preferred stack alignment? AFAIK leaf function doesn't affect >>>> alignment until we finalize it after RA. >>>> >>> >>> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue >>> to be generated in correct form. */ >>> static void >>> ix86_finalize_stack_realign_flags (void) >>> { >>> /* Check if stack realign is really needed after reload, and >>> stores result in cfun */ >>> unsigned int incoming_stack_boundary >>> = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary >>> ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary); >>> unsigned int stack_realign >>> = (incoming_stack_boundary >>> < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor >>> ? crtl->max_used_stack_slot_alignment >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> We call it after RA when all spill slots are allocated and check if we >> may relax stack alignment. Don't see any problem here. > > Please see > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26 > > Why did LRA crash then? Because it tries a patch [1] which doesn't fix stack alignment and STV enabling and therefore doesn't resolve the problem when -mpreferred-stack-boundary=2 is used. Thanks, Ilya -- [1] https://gcc.gnu.org/bugzilla/attachment.cgi?id=37468&action=diff > > > -- > H.J.
On Tue, Feb 2, 2016 at 5:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2016-02-02 16:25 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >> On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>>>>> >>>>>>>>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>>>>>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>>>>>>>> >> to use my patch. >>>>>>>>>>> > >>>>>>>>>>> > Uros, any preferences here? I mean, it is possible to use >>>>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>>>>>>>> > and that is something we don't want to affect. >>>>>>>>>>> >>>>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>>>>>>>> revisit this decision for later gcc versions. >>>>>>>>>>> >>>>>>>>>>> I think disabling STV is less surprising option than increasing stack >>>>>>>>>>> boundary behind the user's back. >>>>>>>>>> >>>>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>>>>>>>> I believe it does just that. >>>>>>>>> >>>>>>>>> This patch is WRONG. >>>>>>>>> >>>>>>>>> -- >>>>>>>>> H.J. >>>>>>>> >>>>>>>> You will run into the same ICE with >>>>>>>> >>>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>>>>>> >>>>>>>> in a leaf function which needs DImode spill/fill. >>>>>>> >>>>>>> Why would we need DImode spill/fill having no DImode registers? >>>>>>> >>>>>> >>>>>> Because STV is enabled with >>>>>> >>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>>> >>>>> I misread it as -mpreferred-... So why would we fail having a proper >>>>> preferred stack alignment? AFAIK leaf function doesn't affect >>>>> alignment until we finalize it after RA. >>>>> >>>> >>>> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue >>>> to be generated in correct form. */ >>>> static void >>>> ix86_finalize_stack_realign_flags (void) >>>> { >>>> /* Check if stack realign is really needed after reload, and >>>> stores result in cfun */ >>>> unsigned int incoming_stack_boundary >>>> = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary >>>> ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary); >>>> unsigned int stack_realign >>>> = (incoming_stack_boundary >>>> < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor >>>> ? crtl->max_used_stack_slot_alignment >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>> >>> We call it after RA when all spill slots are allocated and check if we >>> may relax stack alignment. Don't see any problem here. >> >> Please see >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26 >> >> Why did LRA crash then? > > Because it tries a patch [1] which doesn't fix stack alignment and STV > enabling and therefore doesn't resolve the problem when > -mpreferred-stack-boundary=2 is used. > No, it is because RA doesn't increase stack alignment. You have to have the correct stack alignment requirement before entering RA.
2016-02-02 17:03 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: > On Tue, Feb 2, 2016 at 5:55 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2016-02-02 16:25 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>> On Tue, Feb 2, 2016 at 5:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>> 2016-02-02 16:14 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>> On Tue, Feb 2, 2016 at 5:11 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>>>> 2016-02-02 16:06 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>>>> On Tue, Feb 2, 2016 at 5:03 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>>>>>>> 2016-02-02 15:46 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>: >>>>>>>>> On Tue, Feb 2, 2016 at 4:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>>>>>>>> On Tue, Feb 2, 2016 at 4:29 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>>>>> On Tue, Feb 02, 2016 at 01:24:26PM +0100, Uros Bizjak wrote: >>>>>>>>>>>> On Tue, Feb 2, 2016 at 12:53 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>> >> The bottom line is ix86_minimum_alignment must return the correct >>>>>>>>>>>> >> number for DImode or you can just turn off STV. My suggestion is >>>>>>>>>>>> >> to use my patch. >>>>>>>>>>>> > >>>>>>>>>>>> > Uros, any preferences here? I mean, it is possible to use >>>>>>>>>>>> > e.g. the ix86_option_override_internal and have H.J's ix86_minimum_alignment >>>>>>>>>>>> > change as a safety net, in the usual case for -mpreferred-stack-boundary=2 >>>>>>>>>>>> > we'll just disable TARGET_STV and ix86_minimum_alignment change won't do >>>>>>>>>>>> > anything, as TARGET_STV will be false, and if for whatever case it gets >>>>>>>>>>>> > through (target attribute, -mincoming-stack-boundary=, ...) >>>>>>>>>>>> > ix86_minimum_alignment will be there to ensure enough stack alignment. >>>>>>>>>>>> > Most of the smaller -mpreferred-stack-boundary= uses are -mno-sse anyway, >>>>>>>>>>>> > and that is something we don't want to affect. >>>>>>>>>>>> >>>>>>>>>>>> IMO, we should disable STV when -mpreferred-stack-boundary < 3, as STV >>>>>>>>>>>> is only an optimization. Perhaps we can also emit a "sorry" for >>>>>>>>>>>> explicit -mstv in case stack boundary requirement is not satisfied. >>>>>>>>>>>> *If* there is a need for -mstv with smaller stack boundary, we can >>>>>>>>>>>> revisit this decision for later gcc versions. >>>>>>>>>>>> >>>>>>>>>>>> I think disabling STV is less surprising option than increasing stack >>>>>>>>>>>> boundary behind the user's back. >>>>>>>>>>> >>>>>>>>>>> So, is http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02129.html >>>>>>>>>>> ok for trunk then (alone or with additional sorry, incremental or not?)? >>>>>>>>>>> I believe it does just that. >>>>>>>>>> >>>>>>>>>> This patch is WRONG. >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> H.J. >>>>>>>>> >>>>>>>>> You will run into the same ICE with >>>>>>>>> >>>>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>>>>>>> >>>>>>>>> in a leaf function which needs DImode spill/fill. >>>>>>>> >>>>>>>> Why would we need DImode spill/fill having no DImode registers? >>>>>>>> >>>>>>> >>>>>>> Because STV is enabled with >>>>>>> >>>>>>> -mincoming-stack-boundary=2 -msse2 -O2 -m32 >>>>>> >>>>>> I misread it as -mpreferred-... So why would we fail having a proper >>>>>> preferred stack alignment? AFAIK leaf function doesn't affect >>>>>> alignment until we finalize it after RA. >>>>>> >>>>> >>>>> /* Finalize stack_realign_needed flag, which will guide prologue/epilogue >>>>> to be generated in correct form. */ >>>>> static void >>>>> ix86_finalize_stack_realign_flags (void) >>>>> { >>>>> /* Check if stack realign is really needed after reload, and >>>>> stores result in cfun */ >>>>> unsigned int incoming_stack_boundary >>>>> = (crtl->parm_stack_boundary > ix86_incoming_stack_boundary >>>>> ? crtl->parm_stack_boundary : ix86_incoming_stack_boundary); >>>>> unsigned int stack_realign >>>>> = (incoming_stack_boundary >>>>> < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor >>>>> ? crtl->max_used_stack_slot_alignment >>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> >>>> We call it after RA when all spill slots are allocated and check if we >>>> may relax stack alignment. Don't see any problem here. >>> >>> Please see >>> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69454#c26 >>> >>> Why did LRA crash then? >> >> Because it tries a patch [1] which doesn't fix stack alignment and STV >> enabling and therefore doesn't resolve the problem when >> -mpreferred-stack-boundary=2 is used. >> > > No, it is because RA doesn't increase stack alignment. You have to > have the correct stack alignment requirement before entering RA. And it's too late to do it after STV pass and therefore we disable it when stack is not properly aligned. I think this argumentation goes in a loop. Thanks, Ilya > > > -- > H.J.
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 34b57a4..9fb8db8 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -3588,16 +3588,6 @@ convert_scalars_to_vector () bitmap_obstack_release (NULL); df_process_deferred_rescans (); - /* Conversion means we may have 128bit register spills/fills - which require aligned stack. */ - if (converted_insns) - { - if (crtl->stack_alignment_needed < 128) - crtl->stack_alignment_needed = 128; - if (crtl->stack_alignment_estimated < 128) - crtl->stack_alignment_estimated = 128; - } - return 0; } @@ -5453,6 +5443,11 @@ ix86_option_override_internal (bool main_args_p, opts->x_target_flags |= MASK_VZEROUPPER; if (!(opts_set->x_target_flags & MASK_STV)) opts->x_target_flags |= MASK_STV; + /* Disable STV if -mpreferred-stack-boundary=2 - the needed + stack realignment will be extra cost the pass doesn't take into + account and the pass can't realign the stack. */ + if (ix86_preferred_stack_boundary < 64) + opts->x_target_flags &= ~MASK_STV; if (!ix86_tune_features[X86_TUNE_AVX256_UNALIGNED_LOAD_OPTIMAL] && !(opts_set->x_target_flags & MASK_AVX256_SPLIT_UNALIGNED_LOAD)) opts->x_target_flags |= MASK_AVX256_SPLIT_UNALIGNED_LOAD; diff --git a/gcc/testsuite/gcc.target/i386/pr69454-1.c b/gcc/testsuite/gcc.target/i386/pr69454-1.c new file mode 100644 index 0000000..12ecfd3 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr69454-1.c @@ -0,0 +1,11 @@ +/* { dg-do compile { target { ia32 } } } */ +/* { dg-options "-O2 -msse2 -mno-accumulate-outgoing-args -mpreferred-stack-boundary=2" } */ + +typedef struct { long long w64[2]; } V128; +extern V128* fn2(void); +long long a; +V128 b; +void fn1() { + V128 *c = fn2(); + c->w64[0] = a ^ b.w64[0]; +} diff --git a/gcc/testsuite/gcc.target/i386/pr69454-2.c b/gcc/testsuite/gcc.target/i386/pr69454-2.c new file mode 100644 index 0000000..28bab93 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr69454-2.c @@ -0,0 +1,13 @@ +/* { dg-do compile { target { ia32 } } } */ +/* { dg-options "-O2 -mpreferred-stack-boundary=2" } */ + +extern void fn2 (); +long long a, b; + +void fn1 () +{ + long long c = a; + a = b ^ a; + fn2 (); + a = c; +}