| Message ID | 34204697a62218516a76e6056e791ec0f1aa48eb.camel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFA] Fix various regressions and kernel build failure due to adjust-alignment issue | expand |
On Mon, Jun 8, 2020 at 11:13 PM Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > We're seeing some failures due to the local-alignment pass. For example on sh4: > > Tests that now fail, but worked before (16 tests): > > gcc.dg/pr48335-1.c (test for excess errors) > gcc.dg/pr48335-2.c (test for excess errors) > gcc.dg/pr48335-5.c (test for excess errors) > gcc.dg/pr48335-6.c (test for excess errors) > gcc.dg/torture/pr48493.c -O0 (test for excess errors) > gcc.dg/torture/pr48493.c -O0 (test for excess errors) > gcc.dg/torture/pr48493.c -O1 (test for excess errors) > gcc.dg/torture/pr48493.c -O1 (test for excess errors) > gcc.dg/torture/pr48493.c -O2 (test for excess errors) > gcc.dg/torture/pr48493.c -O2 (test for excess errors) > gcc.dg/torture/pr48493.c -O2 -flto -fno-use-linker-plugin -flto- > partition=none (test for excess errors) > gcc.dg/torture/pr48493.c -O2 -flto -fno-use-linker-plugin -flto- > partition=none (test for excess errors) > gcc.dg/torture/pr48493.c -O3 -g (test for excess errors) > gcc.dg/torture/pr48493.c -O3 -g (test for excess errors) > gcc.dg/torture/pr48493.c -Os (test for excess errors) > gcc.dg/torture/pr48493.c -Os (test for excess errors) > > Or on x86_64: > > during GIMPLE pass: adjust_alignment > /home/jenkins/linux/arch/x86/boot/string.c: In function ‘simple_strtoull’: > /home/jenkins/linux/arch/x86/boot/string.c:121:20: internal compiler error: in > execute, at adjust-alignment.c:74 > 121 | unsigned long long simple_strtoull(const char *cp, char **endp, unsigned > int base) > | ^~~~~~~~~~~~~~~ > 0x79c5b3 execute > ../../../../../gcc/gcc/adjust-alignment.c:74 > Please submit a full bug report, > > There may be others, I haven't searched the failing targets extensively for this > issue. > > AFAICT the adjust-alignment pass is supposed to increase alignments of locals > when needed. It has an assert to ensure that alignments are only increased. > > However, if the object was declared with an alignment attribute that is larger > than what LOCAL_ALIGNMENT would produce for the object, then the adjust-alignment > pass trips the assert. > > Rather than asserting, just ignoring such objects seems better. But I'm not > intimately familiar with the issues here. > > Bootstrapped and regression tested on x86_64, also verified this fixes the sh4 > issue. All the *-elf targets have also built/tested with this patch with no > regressions. > > OK for the trunk? While technically OK the issue is that it does not solve the x86 issue which with incoming stack alignment < 8 bytes does not perform dynamic re-alignment to align 'long long' variables appropriately. Currently the x86 backend thinks it can fixup by lowering alignment via LOCAL_DECL_ALIGNMENT but this is a latent wrong-code issue because the larger alignment is present in the IL for a long (previously RTL expansion, now adjust-aligment) time and your patch makes that wrong info last forever (or alternatively cause dynamic stack alignment which we do _not_ want to perform here). So I prefer to wait for a proper x86 solution before cutting the ICEs. (did you verify effects on code generation of your patch?) Thanks, Richard. > > Jeff > >
On Tue, 2020-06-09 at 09:17 +0200, Richard Biener wrote: > On Mon, Jun 8, 2020 at 11:13 PM Jeff Law via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > We're seeing some failures due to the local-alignment pass. For example on sh4: > > > > Tests that now fail, but worked before (16 tests): > > > > gcc.dg/pr48335-1.c (test for excess errors) > > gcc.dg/pr48335-2.c (test for excess errors) > > gcc.dg/pr48335-5.c (test for excess errors) > > gcc.dg/pr48335-6.c (test for excess errors) > > gcc.dg/torture/pr48493.c -O0 (test for excess errors) > > gcc.dg/torture/pr48493.c -O0 (test for excess errors) > > gcc.dg/torture/pr48493.c -O1 (test for excess errors) > > gcc.dg/torture/pr48493.c -O1 (test for excess errors) > > gcc.dg/torture/pr48493.c -O2 (test for excess errors) > > gcc.dg/torture/pr48493.c -O2 (test for excess errors) > > gcc.dg/torture/pr48493.c -O2 -flto -fno-use-linker-plugin -flto- > > partition=none (test for excess errors) > > gcc.dg/torture/pr48493.c -O2 -flto -fno-use-linker-plugin -flto- > > partition=none (test for excess errors) > > gcc.dg/torture/pr48493.c -O3 -g (test for excess errors) > > gcc.dg/torture/pr48493.c -O3 -g (test for excess errors) > > gcc.dg/torture/pr48493.c -Os (test for excess errors) > > gcc.dg/torture/pr48493.c -Os (test for excess errors) > > > > Or on x86_64: > > > > during GIMPLE pass: adjust_alignment > > /home/jenkins/linux/arch/x86/boot/string.c: In function ‘simple_strtoull’: > > /home/jenkins/linux/arch/x86/boot/string.c:121:20: internal compiler error: in > > execute, at adjust-alignment.c:74 > > 121 | unsigned long long simple_strtoull(const char *cp, char **endp, unsigned > > int base) > > | ^~~~~~~~~~~~~~~ > > 0x79c5b3 execute > > ../../../../../gcc/gcc/adjust-alignment.c:74 > > Please submit a full bug report, > > > > There may be others, I haven't searched the failing targets extensively for this > > issue. > > > > AFAICT the adjust-alignment pass is supposed to increase alignments of locals > > when needed. It has an assert to ensure that alignments are only increased. > > > > However, if the object was declared with an alignment attribute that is larger > > than what LOCAL_ALIGNMENT would produce for the object, then the adjust-alignment > > pass trips the assert. > > > > Rather than asserting, just ignoring such objects seems better. But I'm not > > intimately familiar with the issues here. > > > > Bootstrapped and regression tested on x86_64, also verified this fixes the sh4 > > issue. All the *-elf targets have also built/tested with this patch with no > > regressions. > > > > OK for the trunk? > > While technically OK the issue is that it does not solve the x86 issue > which with incoming stack alignment < 8 bytes does not perform > dynamic re-alignment to align 'long long' variables appropriately. > Currently the x86 backend thinks it can fixup by lowering alignment > via LOCAL_DECL_ALIGNMENT but this is a latent wrong-code issue > because the larger alignment is present in the IL for a long (previously > RTL expansion, now adjust-aligment) time and your patch makes that > wrong info last forever (or alternatively cause dynamic stack alignment > which we do _not_ want to perform here). I've never looked at the dynamic realignment stuff -- is there a good reason why we don't dynamically realign when we've got a local with a requested alignment? Isn't that a huge oversight in the whole concept of dynamic realignment? > > So I prefer to wait for a proper x86 solution before cutting the ICEs. > (did you verify effects on code generation of your patch?) I don't mind waiting. And no, I didn't look at the codegen at all. jeff
On Tue, Jun 09, 2020 at 08:54:47AM -0600, Jeff Law via Gcc-patches wrote: > > While technically OK the issue is that it does not solve the x86 issue > > which with incoming stack alignment < 8 bytes does not perform > > dynamic re-alignment to align 'long long' variables appropriately. > > Currently the x86 backend thinks it can fixup by lowering alignment > > via LOCAL_DECL_ALIGNMENT but this is a latent wrong-code issue > > because the larger alignment is present in the IL for a long (previously > > RTL expansion, now adjust-aligment) time and your patch makes that > > wrong info last forever (or alternatively cause dynamic stack alignment > > which we do _not_ want to perform here). > I've never looked at the dynamic realignment stuff -- is there a good reason why > we don't dynamically realign when we've got a local with a requested alignment? > Isn't that a huge oversight in the whole concept of dynamic realignment? It is quite expensive and long long/double uses are pervasive, so we don't want to realign just because of that. If we do dynamic realignment for other reasons, there is no reason not to align the long long/double properly. Jakub
On Tue, 2020-06-09 at 16:59 +0200, Jakub Jelinek wrote: > On Tue, Jun 09, 2020 at 08:54:47AM -0600, Jeff Law via Gcc-patches wrote: > > > While technically OK the issue is that it does not solve the x86 issue > > > which with incoming stack alignment < 8 bytes does not perform > > > dynamic re-alignment to align 'long long' variables appropriately. > > > Currently the x86 backend thinks it can fixup by lowering alignment > > > via LOCAL_DECL_ALIGNMENT but this is a latent wrong-code issue > > > because the larger alignment is present in the IL for a long (previously > > > RTL expansion, now adjust-aligment) time and your patch makes that > > > wrong info last forever (or alternatively cause dynamic stack alignment > > > which we do _not_ want to perform here). > > I've never looked at the dynamic realignment stuff -- is there a good reason why > > we don't dynamically realign when we've got a local with a requested alignment? > > Isn't that a huge oversight in the whole concept of dynamic realignment? > > It is quite expensive and long long/double uses are pervasive, so we don't > want to realign just because of that. If we do dynamic realignment for > other reasons, there is no reason not to align the long long/double Right, but if there's an explicit alignment from the user, don't we need to honor that? Jeff
On Tue, Jun 09, 2020 at 09:18:25AM -0600, Jeff Law wrote: > On Tue, 2020-06-09 at 16:59 +0200, Jakub Jelinek wrote: > > On Tue, Jun 09, 2020 at 08:54:47AM -0600, Jeff Law via Gcc-patches wrote: > > > > While technically OK the issue is that it does not solve the x86 issue > > > > which with incoming stack alignment < 8 bytes does not perform > > > > dynamic re-alignment to align 'long long' variables appropriately. > > > > Currently the x86 backend thinks it can fixup by lowering alignment > > > > via LOCAL_DECL_ALIGNMENT but this is a latent wrong-code issue > > > > because the larger alignment is present in the IL for a long (previously > > > > RTL expansion, now adjust-aligment) time and your patch makes that > > > > wrong info last forever (or alternatively cause dynamic stack alignment > > > > which we do _not_ want to perform here). > > > I've never looked at the dynamic realignment stuff -- is there a good reason why > > > we don't dynamically realign when we've got a local with a requested alignment? > > > Isn't that a huge oversight in the whole concept of dynamic realignment? > > > > It is quite expensive and long long/double uses are pervasive, so we don't > > want to realign just because of that. If we do dynamic realignment for > > other reasons, there is no reason not to align the long long/double > Right, but if there's an explicit alignment from the user, don't we need to honor > that? Sure. Do we ignore that? For user alignment we have DECL_USER_ALIGN bit... Jakub
On Tue, 2020-06-09 at 17:26 +0200, Jakub Jelinek wrote: > On Tue, Jun 09, 2020 at 09:18:25AM -0600, Jeff Law wrote: > > On Tue, 2020-06-09 at 16:59 +0200, Jakub Jelinek wrote: > > > On Tue, Jun 09, 2020 at 08:54:47AM -0600, Jeff Law via Gcc-patches wrote: > > > > > While technically OK the issue is that it does not solve the x86 issue > > > > > which with incoming stack alignment < 8 bytes does not perform > > > > > dynamic re-alignment to align 'long long' variables appropriately. > > > > > Currently the x86 backend thinks it can fixup by lowering alignment > > > > > via LOCAL_DECL_ALIGNMENT but this is a latent wrong-code issue > > > > > because the larger alignment is present in the IL for a long (previously > > > > > RTL expansion, now adjust-aligment) time and your patch makes that > > > > > wrong info last forever (or alternatively cause dynamic stack alignment > > > > > which we do _not_ want to perform here). > > > > I've never looked at the dynamic realignment stuff -- is there a good reason why > > > > we don't dynamically realign when we've got a local with a requested alignment? > > > > Isn't that a huge oversight in the whole concept of dynamic realignment? > > > > > > It is quite expensive and long long/double uses are pervasive, so we don't > > > want to realign just because of that. If we do dynamic realignment for > > > other reasons, there is no reason not to align the long long/double > > Right, but if there's an explicit alignment from the user, don't we need to honor > > that? > > Sure. Do we ignore that? For user alignment we have DECL_USER_ALIGN bit... I suspect that's what's going on with the kernel build failure. Jeff
On Tue, 9 Jun 2020, Jeff Law via Gcc-patches wrote: > On Tue, 2020-06-09 at 17:26 +0200, Jakub Jelinek wrote: > > On Tue, Jun 09, 2020 at 09:18:25AM -0600, Jeff Law wrote: > > > On Tue, 2020-06-09 at 16:59 +0200, Jakub Jelinek wrote: > > > > On Tue, Jun 09, 2020 at 08:54:47AM -0600, Jeff Law via Gcc-patches wrote: > > > > > > While technically OK the issue is that it does not solve the x86 issue > > > > > > which with incoming stack alignment < 8 bytes does not perform > > > > > > dynamic re-alignment to align 'long long' variables appropriately. > > > > > > Currently the x86 backend thinks it can fixup by lowering alignment > > > > > > via LOCAL_DECL_ALIGNMENT but this is a latent wrong-code issue > > > > > > because the larger alignment is present in the IL for a long (previously > > > > > > RTL expansion, now adjust-aligment) time and your patch makes that > > > > > > wrong info last forever (or alternatively cause dynamic stack alignment > > > > > > which we do _not_ want to perform here). > > > > > I've never looked at the dynamic realignment stuff -- is there a good reason why > > > > > we don't dynamically realign when we've got a local with a requested alignment? > > > > > Isn't that a huge oversight in the whole concept of dynamic realignment? > > > > > > > > It is quite expensive and long long/double uses are pervasive, so we don't > > > > want to realign just because of that. If we do dynamic realignment for > > > > other reasons, there is no reason not to align the long long/double > > > Right, but if there's an explicit alignment from the user, don't we need to honor > > > that? > > > > Sure. Do we ignore that? For user alignment we have DECL_USER_ALIGN bit... > I suspect that's what's going on with the kernel build failure. Sounds like you're about to tackle PR84877. Great! :) brgds, H-P
diff --git a/gcc/adjust-alignment.c b/gcc/adjust-alignment.c index 9b797386bf8..523216a8fcf 100644 --- a/gcc/adjust-alignment.c +++ b/gcc/adjust-alignment.c @@ -71,7 +71,8 @@ pass_adjust_alignment::execute (function *fun) unsigned align = LOCAL_DECL_ALIGNMENT (var); /* Make sure alignment only increase. */ - gcc_assert (align >= DECL_ALIGN (var)); + if (DECL_ALIGN (var) >= align) + continue; SET_DECL_ALIGN (var, align); }
We're seeing some failures due to the local-alignment pass. For example on sh4: Tests that now fail, but worked before (16 tests): gcc.dg/pr48335-1.c (test for excess errors) gcc.dg/pr48335-2.c (test for excess errors) gcc.dg/pr48335-5.c (test for excess errors) gcc.dg/pr48335-6.c (test for excess errors) gcc.dg/torture/pr48493.c -O0 (test for excess errors) gcc.dg/torture/pr48493.c -O0 (test for excess errors) gcc.dg/torture/pr48493.c -O1 (test for excess errors) gcc.dg/torture/pr48493.c -O1 (test for excess errors) gcc.dg/torture/pr48493.c -O2 (test for excess errors) gcc.dg/torture/pr48493.c -O2 (test for excess errors) gcc.dg/torture/pr48493.c -O2 -flto -fno-use-linker-plugin -flto- partition=none (test for excess errors) gcc.dg/torture/pr48493.c -O2 -flto -fno-use-linker-plugin -flto- partition=none (test for excess errors) gcc.dg/torture/pr48493.c -O3 -g (test for excess errors) gcc.dg/torture/pr48493.c -O3 -g (test for excess errors) gcc.dg/torture/pr48493.c -Os (test for excess errors) gcc.dg/torture/pr48493.c -Os (test for excess errors) Or on x86_64: during GIMPLE pass: adjust_alignment /home/jenkins/linux/arch/x86/boot/string.c: In function ‘simple_strtoull’: /home/jenkins/linux/arch/x86/boot/string.c:121:20: internal compiler error: in execute, at adjust-alignment.c:74 121 | unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base) | ^~~~~~~~~~~~~~~ 0x79c5b3 execute ../../../../../gcc/gcc/adjust-alignment.c:74 Please submit a full bug report, There may be others, I haven't searched the failing targets extensively for this issue. AFAICT the adjust-alignment pass is supposed to increase alignments of locals when needed. It has an assert to ensure that alignments are only increased. However, if the object was declared with an alignment attribute that is larger than what LOCAL_ALIGNMENT would produce for the object, then the adjust-alignment pass trips the assert. Rather than asserting, just ignoring such objects seems better. But I'm not intimately familiar with the issues here. Bootstrapped and regression tested on x86_64, also verified this fixes the sh4 issue. All the *-elf targets have also built/tested with this patch with no regressions. OK for the trunk? Jeff