| Message ID | AM6PR03MB51703ACE49CC6C78DCA034FCE4100@AM6PR03MB5170.eurprd03.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR97205 | expand |
On Mon, 2 Nov 2020, Bernd Edlinger wrote: > Hi, > > this makes sure that stack allocated SSA_NAMEs are > at least MODE_ALIGNED. Also increase the MEM_ALIGN > for the corresponding rtl objects. > > > Tested on x86_64-pc-linux-gnu and arm-none-eabi. > > OK for trunk? @@ -1022,6 +1030,14 @@ expand_one_stack_var_at (tree decl, rtx base, unsigned base_align, } set_rtl (decl, x); + + if (TREE_CODE (decl) == SSA_NAME + && GET_MODE (x) != BLKmode + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) + { + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= base_align); + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); + } } I wonder whether we cannot "fix" set_rtl to not call set_mem_attributes in this path, maybe directly call set_mem_align there instead? That is, the preceeding code for ! SSA_NAME already tries to adjust alignment to honor that of the actual stack slot - IMHO the non-SSA and SSA cases should be merged after this patch, but maybe simply by calling set_mem_align instead of doing the DECL_ALIGN frobbing and do the alignment compute also for SSA_NAMEs? The other pieces look OK but the above is a bit ugly at the moment. Thanks, Richard, > > > Thanks > Bernd. >
On 11/2/20 3:07 PM, Richard Biener wrote: > On Mon, 2 Nov 2020, Bernd Edlinger wrote: > >> Hi, >> >> this makes sure that stack allocated SSA_NAMEs are >> at least MODE_ALIGNED. Also increase the MEM_ALIGN >> for the corresponding rtl objects. >> >> >> Tested on x86_64-pc-linux-gnu and arm-none-eabi. >> >> OK for trunk? > > > @@ -1022,6 +1030,14 @@ expand_one_stack_var_at (tree decl, rtx base, > unsigned base_align, > } > > set_rtl (decl, x); > + > + if (TREE_CODE (decl) == SSA_NAME > + && GET_MODE (x) != BLKmode > + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) > + { > + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= > base_align); > + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); > + } > } > > > I wonder whether we cannot "fix" set_rtl to not call > set_mem_attributes in this path, maybe directly call > set_mem_align there instead? That is, the preceeding > code for ! SSA_NAME already tries to adjust alignment > to honor that of the actual stack slot - IMHO the > non-SSA and SSA cases should be merged after this > patch, but maybe simply by calling set_mem_align > instead of doing the DECL_ALIGN frobbing and do > the alignment compute also for SSA_NAMEs? > > The other pieces look OK but the above is a bit ugly > at the moment. > Hmm, how about this? --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -1007,20 +1007,21 @@ expand_one_stack_var_at (tree decl, rtx base, unsigned base_align, x = plus_constant (Pmode, base, offset); x = gen_rtx_MEM (TREE_CODE (decl) == SSA_NAME ? TYPE_MODE (TREE_TYPE (decl)) - : DECL_MODE (SSAVAR (decl)), x); + : DECL_MODE (decl), x); + + /* Set alignment we actually gave this decl if it isn't an SSA name. + If it is we generate stack slots only accidentally so it isn't as + important, we'll simply set the alignment directly on the MEM_P. */ + + if (base == virtual_stack_vars_rtx) + offset -= frame_phase; + align = known_alignment (offset); + align *= BITS_PER_UNIT; + if (align == 0 || align > base_align) + align = base_align; if (TREE_CODE (decl) != SSA_NAME) { - /* Set alignment we actually gave this decl if it isn't an SSA name. - If it is we generate stack slots only accidentally so it isn't as - important, we'll simply use the alignment that is already set. */ - if (base == virtual_stack_vars_rtx) - offset -= frame_phase; - align = known_alignment (offset); - align *= BITS_PER_UNIT; - if (align == 0 || align > base_align) - align = base_align; - /* One would think that we could assert that we're not decreasing alignment here, but (at least) the i386 port does exactly this via the MINIMUM_ALIGNMENT hook. */ @@ -1031,13 +1032,7 @@ expand_one_stack_var_at (tree decl, rtx base, unsigned base_align, set_rtl (decl, x); - if (TREE_CODE (decl) == SSA_NAME - && GET_MODE (x) != BLKmode - && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) - { - gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= base_align); - set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); - } + set_mem_align (x, align); } class stack_vars_data Is it OK if it passes bootstrap and regtesting ? Thanks Bernd. > Thanks, > Richard, > >> >> >> Thanks >> Bernd. >> >
On Mon, 2 Nov 2020, Bernd Edlinger wrote: > On 11/2/20 3:07 PM, Richard Biener wrote: > > On Mon, 2 Nov 2020, Bernd Edlinger wrote: > > > >> Hi, > >> > >> this makes sure that stack allocated SSA_NAMEs are > >> at least MODE_ALIGNED. Also increase the MEM_ALIGN > >> for the corresponding rtl objects. > >> > >> > >> Tested on x86_64-pc-linux-gnu and arm-none-eabi. > >> > >> OK for trunk? > > > > > > @@ -1022,6 +1030,14 @@ expand_one_stack_var_at (tree decl, rtx base, > > unsigned base_align, > > } > > > > set_rtl (decl, x); > > + > > + if (TREE_CODE (decl) == SSA_NAME > > + && GET_MODE (x) != BLKmode > > + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) > > + { > > + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= > > base_align); > > + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); > > + } > > } > > > > > > I wonder whether we cannot "fix" set_rtl to not call > > set_mem_attributes in this path, maybe directly call > > set_mem_align there instead? That is, the preceeding > > code for ! SSA_NAME already tries to adjust alignment > > to honor that of the actual stack slot - IMHO the > > non-SSA and SSA cases should be merged after this > > patch, but maybe simply by calling set_mem_align > > instead of doing the DECL_ALIGN frobbing and do > > the alignment compute also for SSA_NAMEs? > > > > The other pieces look OK but the above is a bit ugly > > at the moment. > > > > Hmm, how about this? That would work for me. Guess removing the DECL_ALIGN frobbing in the != SSA_NAME path didn't work out or you didn't try out of caution? Richard. > --- a/gcc/cfgexpand.c > +++ b/gcc/cfgexpand.c > @@ -1007,20 +1007,21 @@ expand_one_stack_var_at (tree decl, rtx base, unsigned base_align, > x = plus_constant (Pmode, base, offset); > x = gen_rtx_MEM (TREE_CODE (decl) == SSA_NAME > ? TYPE_MODE (TREE_TYPE (decl)) > - : DECL_MODE (SSAVAR (decl)), x); > + : DECL_MODE (decl), x); > + > + /* Set alignment we actually gave this decl if it isn't an SSA name. > + If it is we generate stack slots only accidentally so it isn't as > + important, we'll simply set the alignment directly on the MEM_P. */ > + > + if (base == virtual_stack_vars_rtx) > + offset -= frame_phase; > + align = known_alignment (offset); > + align *= BITS_PER_UNIT; > + if (align == 0 || align > base_align) > + align = base_align; > > if (TREE_CODE (decl) != SSA_NAME) > { > - /* Set alignment we actually gave this decl if it isn't an SSA name. > - If it is we generate stack slots only accidentally so it isn't as > - important, we'll simply use the alignment that is already set. */ > - if (base == virtual_stack_vars_rtx) > - offset -= frame_phase; > - align = known_alignment (offset); > - align *= BITS_PER_UNIT; > - if (align == 0 || align > base_align) > - align = base_align; > - > /* One would think that we could assert that we're not decreasing > alignment here, but (at least) the i386 port does exactly this > via the MINIMUM_ALIGNMENT hook. */ > @@ -1031,13 +1032,7 @@ expand_one_stack_var_at (tree decl, rtx base, unsigned base_align, > > set_rtl (decl, x); > > - if (TREE_CODE (decl) == SSA_NAME > - && GET_MODE (x) != BLKmode > - && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) > - { > - gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= base_align); > - set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); > - } > + set_mem_align (x, align); > } > > class stack_vars_data > > > Is it OK if it passes bootstrap and regtesting ? > > Thanks > Bernd. > > > Thanks, > > Richard, > > > >> > >> > >> Thanks > >> Bernd. > >> > > >
On 11/3/20 10:34 AM, Richard Biener wrote: > On Mon, 2 Nov 2020, Bernd Edlinger wrote: > >> On 11/2/20 3:07 PM, Richard Biener wrote: >>> On Mon, 2 Nov 2020, Bernd Edlinger wrote: >>> >>>> Hi, >>>> >>>> this makes sure that stack allocated SSA_NAMEs are >>>> at least MODE_ALIGNED. Also increase the MEM_ALIGN >>>> for the corresponding rtl objects. >>>> >>>> >>>> Tested on x86_64-pc-linux-gnu and arm-none-eabi. >>>> >>>> OK for trunk? >>> >>> >>> @@ -1022,6 +1030,14 @@ expand_one_stack_var_at (tree decl, rtx base, >>> unsigned base_align, >>> } >>> >>> set_rtl (decl, x); >>> + >>> + if (TREE_CODE (decl) == SSA_NAME >>> + && GET_MODE (x) != BLKmode >>> + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) >>> + { >>> + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= >>> base_align); >>> + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); >>> + } >>> } >>> >>> >>> I wonder whether we cannot "fix" set_rtl to not call >>> set_mem_attributes in this path, maybe directly call >>> set_mem_align there instead? That is, the preceeding >>> code for ! SSA_NAME already tries to adjust alignment >>> to honor that of the actual stack slot - IMHO the >>> non-SSA and SSA cases should be merged after this >>> patch, but maybe simply by calling set_mem_align >>> instead of doing the DECL_ALIGN frobbing and do >>> the alignment compute also for SSA_NAMEs? >>> >>> The other pieces look OK but the above is a bit ugly >>> at the moment. >>> >> >> Hmm, how about this? > > That would work for me. Guess removing the DECL_ALIGN frobbing > in the != SSA_NAME path didn't work out or you didn't try out > of caution? > I didn't try, since it felt simply more correct this way, and get_object_alignment would probably give a different answer since it uses DECL_ALIGN too. Bernd.
On Tue, 3 Nov 2020, Bernd Edlinger wrote: > > > On 11/3/20 10:34 AM, Richard Biener wrote: > > On Mon, 2 Nov 2020, Bernd Edlinger wrote: > > > >> On 11/2/20 3:07 PM, Richard Biener wrote: > >>> On Mon, 2 Nov 2020, Bernd Edlinger wrote: > >>> > >>>> Hi, > >>>> > >>>> this makes sure that stack allocated SSA_NAMEs are > >>>> at least MODE_ALIGNED. Also increase the MEM_ALIGN > >>>> for the corresponding rtl objects. > >>>> > >>>> > >>>> Tested on x86_64-pc-linux-gnu and arm-none-eabi. > >>>> > >>>> OK for trunk? > >>> > >>> > >>> @@ -1022,6 +1030,14 @@ expand_one_stack_var_at (tree decl, rtx base, > >>> unsigned base_align, > >>> } > >>> > >>> set_rtl (decl, x); > >>> + > >>> + if (TREE_CODE (decl) == SSA_NAME > >>> + && GET_MODE (x) != BLKmode > >>> + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) > >>> + { > >>> + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= > >>> base_align); > >>> + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); > >>> + } > >>> } > >>> > >>> > >>> I wonder whether we cannot "fix" set_rtl to not call > >>> set_mem_attributes in this path, maybe directly call > >>> set_mem_align there instead? That is, the preceeding > >>> code for ! SSA_NAME already tries to adjust alignment > >>> to honor that of the actual stack slot - IMHO the > >>> non-SSA and SSA cases should be merged after this > >>> patch, but maybe simply by calling set_mem_align > >>> instead of doing the DECL_ALIGN frobbing and do > >>> the alignment compute also for SSA_NAMEs? > >>> > >>> The other pieces look OK but the above is a bit ugly > >>> at the moment. > >>> > >> > >> Hmm, how about this? > > > > That would work for me. Guess removing the DECL_ALIGN frobbing > > in the != SSA_NAME path didn't work out or you didn't try out > > of caution? > > > > I didn't try, since it felt simply more correct this way, > and get_object_alignment would probably give a different > answer since it uses DECL_ALIGN too. OK, I see. Richard.
On 11/3/20 11:16 AM, Richard Biener wrote: > On Tue, 3 Nov 2020, Bernd Edlinger wrote: > >> >> >> On 11/3/20 10:34 AM, Richard Biener wrote: >>> On Mon, 2 Nov 2020, Bernd Edlinger wrote: >>> >>>> On 11/2/20 3:07 PM, Richard Biener wrote: >>>>> On Mon, 2 Nov 2020, Bernd Edlinger wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> this makes sure that stack allocated SSA_NAMEs are >>>>>> at least MODE_ALIGNED. Also increase the MEM_ALIGN >>>>>> for the corresponding rtl objects. >>>>>> >>>>>> >>>>>> Tested on x86_64-pc-linux-gnu and arm-none-eabi. >>>>>> >>>>>> OK for trunk? >>>>> >>>>> >>>>> @@ -1022,6 +1030,14 @@ expand_one_stack_var_at (tree decl, rtx base, >>>>> unsigned base_align, >>>>> } >>>>> >>>>> set_rtl (decl, x); >>>>> + >>>>> + if (TREE_CODE (decl) == SSA_NAME >>>>> + && GET_MODE (x) != BLKmode >>>>> + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) >>>>> + { >>>>> + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= >>>>> base_align); >>>>> + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); >>>>> + } >>>>> } >>>>> >>>>> >>>>> I wonder whether we cannot "fix" set_rtl to not call >>>>> set_mem_attributes in this path, maybe directly call >>>>> set_mem_align there instead? That is, the preceeding >>>>> code for ! SSA_NAME already tries to adjust alignment >>>>> to honor that of the actual stack slot - IMHO the >>>>> non-SSA and SSA cases should be merged after this >>>>> patch, but maybe simply by calling set_mem_align >>>>> instead of doing the DECL_ALIGN frobbing and do >>>>> the alignment compute also for SSA_NAMEs? >>>>> >>>>> The other pieces look OK but the above is a bit ugly >>>>> at the moment. >>>>> >>>> >>>> Hmm, how about this? >>> >>> That would work for me. Guess removing the DECL_ALIGN frobbing >>> in the != SSA_NAME path didn't work out or you didn't try out >>> of caution? >>> >> >> I didn't try, since it felt simply more correct this way, >> and get_object_alignment would probably give a different >> answer since it uses DECL_ALIGN too. > > OK, I see. > > Richard. > Ok, good. So this is what I will commit shortly. Thanks Bernd.
From e8f80c0ed18c49e8b8ad4145401a2c1afa50af60 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Sun, 1 Nov 2020 07:32:20 +0100 Subject: [PATCH] Fix PR97205 This makes sure that stack allocated SSA_NAMEs are at least MODE_ALIGNED. Also increase the MEM_ALIGN for the corresponding rtl objects. gcc: 2020-11-01 Bernd Edlinger <bernd.edlinger@hotmail.de> PR target/97205 * cfgexpand.c (align_local_variable): Make SSA_NAMEs at least MODE_ALIGNED. (expand_one_stack_var_at): Increase MEM_ALIGN for SSA_NAMEs. testsuite: 2020-11-01 Bernd Edlinger <bernd.edlinger@hotmail.de> PR target/97205 * gcc.c-torture/compile/pr97205.c: New test. --- gcc/cfgexpand.c | 26 ++++++++++++++++++++------ gcc/testsuite/gcc.c-torture/compile/pr97205.c | 7 +++++++ 2 files changed, 27 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr97205.c diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index f3f17d3..3aec250 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -366,7 +366,15 @@ align_local_variable (tree decl, bool really_expand) unsigned int align; if (TREE_CODE (decl) == SSA_NAME) - align = TYPE_ALIGN (TREE_TYPE (decl)); + { + tree type = TREE_TYPE (decl); + machine_mode mode = TYPE_MODE (type); + + align = TYPE_ALIGN (type); + if (mode != BLKmode + && align < GET_MODE_ALIGNMENT (mode)) + align = GET_MODE_ALIGNMENT (mode); + } else { align = LOCAL_DECL_ALIGNMENT (decl); @@ -1022,6 +1030,14 @@ expand_one_stack_var_at (tree decl, rtx base, unsigned base_align, } set_rtl (decl, x); + + if (TREE_CODE (decl) == SSA_NAME + && GET_MODE (x) != BLKmode + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x))) + { + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= base_align); + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); + } } class stack_vars_data @@ -1327,13 +1343,11 @@ expand_one_stack_var_1 (tree var) { tree type = TREE_TYPE (var); size = tree_to_poly_uint64 (TYPE_SIZE_UNIT (type)); - byte_align = TYPE_ALIGN_UNIT (type); } else - { - size = tree_to_poly_uint64 (DECL_SIZE_UNIT (var)); - byte_align = align_local_variable (var, true); - } + size = tree_to_poly_uint64 (DECL_SIZE_UNIT (var)); + + byte_align = align_local_variable (var, true); /* We handle highly aligned variables in expand_stack_vars. */ gcc_assert (byte_align * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT); diff --git a/gcc/testsuite/gcc.c-torture/compile/pr97205.c b/gcc/testsuite/gcc.c-torture/compile/pr97205.c new file mode 100644 index 0000000..6600011 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr97205.c @@ -0,0 +1,7 @@ +int a; +typedef __attribute__((aligned(2))) int x; +int f () +{ + x b = a; + return b; +} -- 1.9.1