Fix PR middle-end/86705

Message ID 428eb6a9-a647-b46c-6b8f-c7456773dacd@mittosystems.com
State New
Headers show
Series
  • Fix PR middle-end/86705
Related show

Commit Message

Jozef Lawrynowicz July 29, 2018, 4:27 p.m.
pr45678-2.c ICEs for msp430-elf with -mlarge, because an alignment of
POINTER_SIZE is attempted. POINTER_SIZE with -mlarge is 20-bits, so further
code in the middle-end that expects this to be a power or 2 causes odd
alignments to be set, in this case eventually resulting in an ICE.

The test ICEs on gcc-7-branch, gcc-8-branch, and current trunk. It
successfully builds on gcc-6-branch.
The failure is caused by r235172.

Successfully bootstrapped and regtested the attached patch for
x86-64-pc-linux-gnu, and msp430-elf with -mlarge, on trunk.

Ok for gcc-7-branch, gcc-8-branch and trunk?

Comments

Richard Biener July 30, 2018, 1:29 p.m. | #1
On Sun, Jul 29, 2018 at 6:27 PM Jozef Lawrynowicz
<jozef.l@mittosystems.com> wrote:
>

> pr45678-2.c ICEs for msp430-elf with -mlarge, because an alignment of

> POINTER_SIZE is attempted. POINTER_SIZE with -mlarge is 20-bits, so further

> code in the middle-end that expects this to be a power or 2 causes odd

> alignments to be set, in this case eventually resulting in an ICE.

>

> The test ICEs on gcc-7-branch, gcc-8-branch, and current trunk. It

> successfully builds on gcc-6-branch.

> The failure is caused by r235172.

>

> Successfully bootstrapped and regtested the attached patch for

> x86-64-pc-linux-gnu, and msp430-elf with -mlarge, on trunk.

>

> Ok for gcc-7-branch, gcc-8-branch and trunk?


I wonder if most (if not all) places you touch want to use
get_mode_alignment (Pmode) instead?  (or ptr_mode)

Anyhow, the patch is otherwise obvious though factoring
the thing might be nice (thus my suggestion above...)

Richard.
Jozef Lawrynowicz July 31, 2018, 6:37 p.m. | #2
On 30/07/18 14:29, Richard Biener wrote:
> On Sun, Jul 29, 2018 at 6:27 PM Jozef Lawrynowicz

> <jozef.l@mittosystems.com> wrote:

>> pr45678-2.c ICEs for msp430-elf with -mlarge, because an alignment of

>> POINTER_SIZE is attempted. POINTER_SIZE with -mlarge is 20-bits, so further

>> code in the middle-end that expects this to be a power or 2 causes odd

>> alignments to be set, in this case eventually resulting in an ICE.

>>

>> The test ICEs on gcc-7-branch, gcc-8-branch, and current trunk. It

>> successfully builds on gcc-6-branch.

>> The failure is caused by r235172.

>>

>> Successfully bootstrapped and regtested the attached patch for

>> x86-64-pc-linux-gnu, and msp430-elf with -mlarge, on trunk.

>>

>> Ok for gcc-7-branch, gcc-8-branch and trunk?

> I wonder if most (if not all) places you touch want to use

> get_mode_alignment (Pmode) instead?  (or ptr_mode)

>

> Anyhow, the patch is otherwise obvious though factoring

> the thing might be nice (thus my suggestion above...)

>

> Richard.


Thanks for the suggestion, using GET_MODE_ALIGNMENT does seem like a neater
idea.
After retesting, I went ahead and committed the below patch onto trunk, will
backport to gcc-7/8-branch later.
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index d6e3c38..7353d5d 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1257,10 +1257,10 @@ set_parm_rtl (tree parm, rtx x)
 	 allocate it, which means that in-frame portion is just a
 	 pointer.  ??? We've got a pseudo for sure here, do we
 	 actually dynamically allocate its spilling area if needed?
-	 ??? Isn't it a problem when POINTER_SIZE also exceeds
-	 MAX_SUPPORTED_STACK_ALIGNMENT, as on cris and lm32?  */
+	 ??? Isn't it a problem when Pmode alignment also exceeds
+	 MAX_SUPPORTED_STACK_ALIGNMENT, as can happen on cris and lm32?  */
       if (align > MAX_SUPPORTED_STACK_ALIGNMENT)
-	align = POINTER_SIZE;
+	align = GET_MODE_ALIGNMENT (Pmode);
 
       record_alignment_for_reg_var (align);
     }
@@ -1381,7 +1381,7 @@ expand_one_ssa_partition (tree var)
   /* If the variable alignment is very large we'll dynamicaly allocate
      it, which means that in-frame portion is just a pointer.  */
   if (align > MAX_SUPPORTED_STACK_ALIGNMENT)
-    align = POINTER_SIZE;
+    align = GET_MODE_ALIGNMENT (Pmode);
 
   record_alignment_for_reg_var (align);
 
@@ -1608,7 +1608,7 @@ expand_one_var (tree var, bool toplevel, bool really_expand)
       /* If the variable alignment is very large we'll dynamicaly allocate
 	 it, which means that in-frame portion is just a pointer.  */
       if (align > MAX_SUPPORTED_STACK_ALIGNMENT)
-	align = POINTER_SIZE;
+	align = GET_MODE_ALIGNMENT (Pmode);
     }
 
   record_alignment_for_reg_var (align);

Patch

From e655a518a06a848dc398504f28272750e1a2be9f Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozefl.opensrc@gmail.com>
Date: Tue, 13 Mar 2018 11:25:56 +0000
Subject: [PATCH] When aligning to POINTER_SIZE, round alignment to largest
 power of 2 that is <= POINTER_SIZE

	PR middle-end/86705
	* gcc/cfgexpand.c (set_parm_rtl): Before using POINTER_SIZE as an
	alignment value, round it to the largest power of 2 less than or equal
	to itself.
	(expand_one_ssa_partition): Likewise.
	(expand_one_var): Likewise.

---
 gcc/cfgexpand.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index d6e3c38..a56db7a 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1258,9 +1258,12 @@  set_parm_rtl (tree parm, rtx x)
 	 pointer.  ??? We've got a pseudo for sure here, do we
 	 actually dynamically allocate its spilling area if needed?
 	 ??? Isn't it a problem when POINTER_SIZE also exceeds
-	 MAX_SUPPORTED_STACK_ALIGNMENT, as on cris and lm32?  */
+	 MAX_SUPPORTED_STACK_ALIGNMENT, as on cris and lm32?
+	 POINTER_SIZE may not be a power of 2 e.g. for msp430-elf with the large
+	 data model, so align to the largest power of 2 that is
+	 <= POINTER_SIZE.  */
       if (align > MAX_SUPPORTED_STACK_ALIGNMENT)
-	align = POINTER_SIZE;
+	align = (unsigned)1 << floor_log2 (POINTER_SIZE);
 
       record_alignment_for_reg_var (align);
     }
@@ -1381,7 +1384,7 @@  expand_one_ssa_partition (tree var)
   /* If the variable alignment is very large we'll dynamicaly allocate
      it, which means that in-frame portion is just a pointer.  */
   if (align > MAX_SUPPORTED_STACK_ALIGNMENT)
-    align = POINTER_SIZE;
+    align = (unsigned)1 << floor_log2 (POINTER_SIZE);
 
   record_alignment_for_reg_var (align);
 
@@ -1608,7 +1611,7 @@  expand_one_var (tree var, bool toplevel, bool really_expand)
       /* If the variable alignment is very large we'll dynamicaly allocate
 	 it, which means that in-frame portion is just a pointer.  */
       if (align > MAX_SUPPORTED_STACK_ALIGNMENT)
-	align = POINTER_SIZE;
+	align = (unsigned)1 << floor_log2 (POINTER_SIZE);
     }
 
   record_alignment_for_reg_var (align);
-- 
2.7.4