[RFA] Fix various regressions and kernel build failure due to adjust-alignment issue

Message ID 34204697a62218516a76e6056e791ec0f1aa48eb.camel@redhat.com
State New
Headers show
Series
  • [RFA] Fix various regressions and kernel build failure due to adjust-alignment issue
Related show

Commit Message

Christophe Lyon via Gcc-patches June 8, 2020, 9:12 p.m.
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

Comments

Christophe Lyon via Gcc-patches June 9, 2020, 7:17 a.m. | #1
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

>

>
Christophe Lyon via Gcc-patches June 9, 2020, 2:54 p.m. | #2
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
Christophe Lyon via Gcc-patches June 9, 2020, 2:59 p.m. | #3
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
Christophe Lyon via Gcc-patches June 9, 2020, 3:18 p.m. | #4
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
Christophe Lyon via Gcc-patches June 9, 2020, 3:26 p.m. | #5
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
Christophe Lyon via Gcc-patches June 9, 2020, 3:27 p.m. | #6
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
Hans-Peter Nilsson June 11, 2020, 7:36 p.m. | #7
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

Patch

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);
     }