Merge lp:~michaelh1/gcc-linaro/lp723185 into lp:gcc-linaro/4.6

Proposed by Michael Hope
Status: Merged
Approved by: Richard Sandiford
Approved revision: no longer in the source branch.
Merged at revision: 106761
Proposed branch: lp:~michaelh1/gcc-linaro/lp723185
Merge into: lp:gcc-linaro/4.6
Diff against target: 197 lines (+150/-18)
3 files modified
ChangeLog.linaro (+21/-0)
gcc/config/arm/arm.c (+128/-17)
gcc/config/arm/arm.md (+1/-1)
To merge this branch: bzr merge lp:~michaelh1/gcc-linaro/lp723185
Reviewer Review Type Date Requested Status
Richard Sandiford Approve
Review via email: mp+63965@code.launchpad.net

This proposal supersedes a proposal from 2011-06-08.

Description of the change

Backport r172297 from trunk to fix LP: #723185. Now pulls in the offset limit check.

To post a comment you must log in.
Revision history for this message
Linaro Toolchain Builder (cbuild) wrote : Posted in a previous version of this proposal

cbuild has taken a snapshot of this branch at r106755 and queued it for build.

The snapshot is available at:
 http://ex.seabright.co.nz/snapshots/gcc-linaro-4.6+bzr106755~michaelh1~lp723185.tar.xdelta3.xz

and will be built on the following builders:
 a9-builder i686 x86_64

You can track the build queue at:
 http://ex.seabright.co.nz/helpers/scheduler

cbuild-snapshot: gcc-linaro-4.6+bzr106755~michaelh1~lp723185
cbuild-ancestor: lp:gcc-linaro/4.6+bzr106754
cbuild-state: check

Revision history for this message
Linaro Toolchain Builder (cbuild) wrote : Posted in a previous version of this proposal

cbuild successfully built this on i686-lucid-cbuild127-scorpius-i686r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106755~michaelh1~lp723185/logs/i686-lucid-cbuild127-scorpius-i686r1

The test suite results were unchanged compared to the branch point lp:gcc-linaro/4.6+bzr106754.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106755~michaelh1~lp723185/logs/i686-lucid-cbuild127-scorpius-i686r1/gcc-testsuite.txt

cbuild-checked: i686-lucid-cbuild127-scorpius-i686r1

Revision history for this message
Linaro Toolchain Builder (cbuild) wrote : Posted in a previous version of this proposal

cbuild successfully built this on x86_64-maverick-cbuild127-crucis-x86_64r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106755~michaelh1~lp723185/logs/x86_64-maverick-cbuild127-crucis-x86_64r1

The test suite results were unchanged compared to the branch point lp:gcc-linaro/4.6+bzr106754.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106755~michaelh1~lp723185/logs/x86_64-maverick-cbuild127-crucis-x86_64r1/gcc-testsuite.txt

cbuild-checked: x86_64-maverick-cbuild127-crucis-x86_64r1

Revision history for this message
Richard Sandiford (rsandifo) wrote : Posted in a previous version of this proposal

I think you also need:

2011-04-20 Chung-Lin Tang <email address hidden>

 * config/arm/arm.c (arm_legitimize_reload_address): For NEON
 quad-word modes, reduce to 9-bit index range when above 1016 limit.

(fixes http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01635.html )

review: Needs Fixing
Revision history for this message
Ramana Radhakrishnan (ramana) wrote : Posted in a previous version of this proposal

On 9 June 2011 09:07, Richard Sandiford <email address hidden> wrote:
> Review: Needs Fixing
> I think you also need:

FTR, I had tried to backport the above mentioned fix to the FSF 4.6
branch and saw a few ICEs in the testsuite that I've posted about at
http://gcc.gnu.org/PR48250

cheers
Ramana

>
> 2011-04-20  Chung-Lin Tang  <email address hidden>
>
>        * config/arm/arm.c (arm_legitimize_reload_address): For NEON
>        quad-word modes, reduce to 9-bit index range when above 1016 limit.
>
> (fixes http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01635.html )
>
>
> --
> https://code.launchpad.net/~michaelh1/gcc-linaro/lp723185/+merge/63937
> Your team Linaro Toolchain Developers is subscribed to branch lp:gcc-linaro/4.6.
>

Revision history for this message
Michael Hope (michaelh1) wrote :

I'm running this to see if the ICEs that Ramana saw occur.

Revision history for this message
Linaro Toolchain Builder (cbuild) wrote :

cbuild has taken a snapshot of this branch at r106756 and queued it for build.

The snapshot is available at:
 http://ex.seabright.co.nz/snapshots/gcc-linaro-4.6+bzr106756~michaelh1~lp723185.tar.xdelta3.xz

and will be built on the following builders:
 a9-builder i686 x86_64

You can track the build queue at:
 http://ex.seabright.co.nz/helpers/scheduler

cbuild-snapshot: gcc-linaro-4.6+bzr106756~michaelh1~lp723185
cbuild-ancestor: lp:gcc-linaro/4.6+bzr106754
cbuild-state: check

Revision history for this message
Linaro Toolchain Builder (cbuild) wrote :

cbuild successfully built this on x86_64-maverick-cbuild127-crucis-x86_64r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/x86_64-maverick-cbuild127-crucis-x86_64r1

The test suite results were unchanged compared to the branch point lp:gcc-linaro/4.6+bzr106754.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/x86_64-maverick-cbuild127-crucis-x86_64r1/gcc-testsuite.txt

cbuild-checked: x86_64-maverick-cbuild127-crucis-x86_64r1

Revision history for this message
Linaro Toolchain Builder (cbuild) wrote :

cbuild successfully built this on armv7l-maverick-cbuild127-ursa4-cortexa9r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/armv7l-maverick-cbuild127-ursa4-cortexa9r1

The test suite results were unchanged compared to the branch point lp:gcc-linaro/4.6+bzr106754.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/armv7l-maverick-cbuild127-ursa4-cortexa9r1/gcc-testsuite.txt

cbuild-checked: armv7l-maverick-cbuild127-ursa4-cortexa9r1

Revision history for this message
Linaro Toolchain Builder (cbuild) wrote :

cbuild successfully built this on i686-lucid-cbuild127-scorpius-i686r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/i686-lucid-cbuild127-scorpius-i686r1

The test suite results were unchanged compared to the branch point lp:gcc-linaro/4.6+bzr106754.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/i686-lucid-cbuild127-scorpius-i686r1/gcc-testsuite.txt

cbuild-checked: i686-lucid-cbuild127-scorpius-i686r1

Revision history for this message
Michael Hope (michaelh1) wrote :

The -mthumb -mfpu=neon build bootstrapped fine with no regressions. I'm running a -marm -mfpu=neon and -mthumb -fpu=vfpv3-d16 as well.

Revision history for this message
Chung-Lin Tang (cltang) wrote :

On 2011/6/10 07:51 AM, Michael Hope wrote:
> The -mthumb -mfpu=neon build bootstrapped fine with no regressions. I'm running a -marm -mfpu=neon and -mthumb -fpu=vfpv3-d16 as well.

This patch actually only affects ARM mode. The original ICE should not
have occurred in Thumb-2, IIRC.

Revision history for this message
Michael Hope (michaelh1) wrote :

Cortex-A9+NEON+ARM bootstraps fine. The testsuite comes out cleaner due to being in ARM mode.

Cortex-A8+VFP+Thumb2 bootstraps fine. The testsuite seems OK, but it's hard to tell due to the NEON vs non-NEON differences.

Revision history for this message
Linaro Toolchain Builder (cbuild) wrote :

cbuild successfully built this on armv7l-maverick-cbuild128-ursa3-cortexa8r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/armv7l-maverick-cbuild128-ursa3-cortexa8r1

The test suite was not checked as the branch point lp:gcc-linaro/4.6+bzr106754 has nothing to compare against.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/armv7l-maverick-cbuild128-ursa3-cortexa8r1/gcc-testsuite.txt

cbuild-checked: armv7l-maverick-cbuild128-ursa3-cortexa8r1

Revision history for this message
Linaro Toolchain Builder (cbuild) wrote :

cbuild successfully built this on armv7l-maverick-cbuild128-ursa4-cortexa9armr1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/armv7l-maverick-cbuild128-ursa4-cortexa9armr1

The test suite was not checked as the branch point lp:gcc-linaro/4.6+bzr106754 has nothing to compare against.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.6+bzr106756~michaelh1~lp723185/logs/armv7l-maverick-cbuild128-ursa4-cortexa9armr1/gcc-testsuite.txt

cbuild-checked: armv7l-maverick-cbuild128-ursa4-cortexa9armr1

Revision history for this message
Michael Hope (michaelh1) wrote :

Any thoughts on this? GCC bootstraps in Thumb-2 NEON, Thumb-2 VFP, and ARM NEON modes.

Revision history for this message
Richard Sandiford (rsandifo) wrote :

Looks good. Sorry for the slow response.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog.linaro'
2--- ChangeLog.linaro 2011-06-07 11:18:20 +0000
3+++ ChangeLog.linaro 2011-06-09 09:02:19 +0000
4@@ -1,3 +1,24 @@
5+2011-06-09 Michael Hope <michael.hope@linaro.org>
6+
7+ gcc/
8+ Backport from mainline:
9+
10+ Chung-Lin Tang <cltang@codesourcery.com>
11+ Richard Earnshaw <rearnsha@arm.com>
12+
13+ PR target/48250
14+ * config/arm/arm.c (arm_legitimize_reload_address): Update cases
15+ to use sign-magnitude offsets. Reject unsupported unaligned
16+ cases. Add detailed description in comments.
17+ * config/arm/arm.md (reload_outdf): Disable for ARM mode; change
18+ condition from TARGET_32BIT to TARGET_ARM.
19+
20+ Chung-Lin Tang <cltang@codesourcery.com>
21+
22+ * config/arm/arm.c (arm_legitimize_reload_address): For NEON
23+ quad-word modes, reduce to 9-bit index range when above 1016
24+ limit.
25+
26 2011-06-07 Andrew Stubbs <ams@codesourcery.com>
27
28 Backport from FSF:
29
30=== modified file 'gcc/config/arm/arm.c'
31--- gcc/config/arm/arm.c 2011-05-11 14:49:48 +0000
32+++ gcc/config/arm/arm.c 2011-06-09 09:02:19 +0000
33@@ -6419,23 +6419,134 @@
34 HOST_WIDE_INT val = INTVAL (XEXP (*p, 1));
35 HOST_WIDE_INT low, high;
36
37- if (mode == DImode || (mode == DFmode && TARGET_SOFT_FLOAT))
38- low = ((val & 0xf) ^ 0x8) - 0x8;
39- else if (TARGET_MAVERICK && TARGET_HARD_FLOAT)
40- /* Need to be careful, -256 is not a valid offset. */
41- low = val >= 0 ? (val & 0xff) : -((-val) & 0xff);
42- else if (mode == SImode
43- || (mode == SFmode && TARGET_SOFT_FLOAT)
44- || ((mode == HImode || mode == QImode) && ! arm_arch4))
45- /* Need to be careful, -4096 is not a valid offset. */
46- low = val >= 0 ? (val & 0xfff) : -((-val) & 0xfff);
47- else if ((mode == HImode || mode == QImode) && arm_arch4)
48- /* Need to be careful, -256 is not a valid offset. */
49- low = val >= 0 ? (val & 0xff) : -((-val) & 0xff);
50- else if (GET_MODE_CLASS (mode) == MODE_FLOAT
51- && TARGET_HARD_FLOAT && TARGET_FPA)
52- /* Need to be careful, -1024 is not a valid offset. */
53- low = val >= 0 ? (val & 0x3ff) : -((-val) & 0x3ff);
54+ /* Detect coprocessor load/stores. */
55+ bool coproc_p = ((TARGET_HARD_FLOAT
56+ && (TARGET_VFP || TARGET_FPA || TARGET_MAVERICK)
57+ && (mode == SFmode || mode == DFmode
58+ || (mode == DImode && TARGET_MAVERICK)))
59+ || (TARGET_REALLY_IWMMXT
60+ && VALID_IWMMXT_REG_MODE (mode))
61+ || (TARGET_NEON
62+ && (VALID_NEON_DREG_MODE (mode)
63+ || VALID_NEON_QREG_MODE (mode))));
64+
65+ /* For some conditions, bail out when lower two bits are unaligned. */
66+ if ((val & 0x3) != 0
67+ /* Coprocessor load/store indexes are 8-bits + '00' appended. */
68+ && (coproc_p
69+ /* For DI, and DF under soft-float: */
70+ || ((mode == DImode || mode == DFmode)
71+ /* Without ldrd, we use stm/ldm, which does not
72+ fair well with unaligned bits. */
73+ && (! TARGET_LDRD
74+ /* Thumb-2 ldrd/strd is [-1020,+1020] in steps of 4. */
75+ || TARGET_THUMB2))))
76+ return false;
77+
78+ /* When breaking down a [reg+index] reload address into [(reg+high)+low],
79+ of which the (reg+high) gets turned into a reload add insn,
80+ we try to decompose the index into high/low values that can often
81+ also lead to better reload CSE.
82+ For example:
83+ ldr r0, [r2, #4100] // Offset too large
84+ ldr r1, [r2, #4104] // Offset too large
85+
86+ is best reloaded as:
87+ add t1, r2, #4096
88+ ldr r0, [t1, #4]
89+ add t2, r2, #4096
90+ ldr r1, [t2, #8]
91+
92+ which post-reload CSE can simplify in most cases to eliminate the
93+ second add instruction:
94+ add t1, r2, #4096
95+ ldr r0, [t1, #4]
96+ ldr r1, [t1, #8]
97+
98+ The idea here is that we want to split out the bits of the constant
99+ as a mask, rather than as subtracting the maximum offset that the
100+ respective type of load/store used can handle.
101+
102+ When encountering negative offsets, we can still utilize it even if
103+ the overall offset is positive; sometimes this may lead to an immediate
104+ that can be constructed with fewer instructions.
105+ For example:
106+ ldr r0, [r2, #0x3FFFFC]
107+
108+ This is best reloaded as:
109+ add t1, r2, #0x400000
110+ ldr r0, [t1, #-4]
111+
112+ The trick for spotting this for a load insn with N bits of offset
113+ (i.e. bits N-1:0) is to look at bit N; if it is set, then chose a
114+ negative offset that is going to make bit N and all the bits below
115+ it become zero in the remainder part.
116+
117+ The SIGN_MAG_LOW_ADDR_BITS macro below implements this, with respect
118+ to sign-magnitude addressing (i.e. separate +- bit, or 1's complement),
119+ used in most cases of ARM load/store instructions. */
120+
121+#define SIGN_MAG_LOW_ADDR_BITS(VAL, N) \
122+ (((VAL) & ((1 << (N)) - 1)) \
123+ ? (((VAL) & ((1 << ((N) + 1)) - 1)) ^ (1 << (N))) - (1 << (N)) \
124+ : 0)
125+
126+ if (coproc_p)
127+ {
128+ low = SIGN_MAG_LOW_ADDR_BITS (val, 10);
129+
130+ /* NEON quad-word load/stores are made of two double-word accesses,
131+ so the valid index range is reduced by 8. Treat as 9-bit range if
132+ we go over it. */
133+ if (TARGET_NEON && VALID_NEON_QREG_MODE (mode) && low >= 1016)
134+ low = SIGN_MAG_LOW_ADDR_BITS (val, 9);
135+ }
136+ else if (GET_MODE_SIZE (mode) == 8)
137+ {
138+ if (TARGET_LDRD)
139+ low = (TARGET_THUMB2
140+ ? SIGN_MAG_LOW_ADDR_BITS (val, 10)
141+ : SIGN_MAG_LOW_ADDR_BITS (val, 8));
142+ else
143+ /* For pre-ARMv5TE (without ldrd), we use ldm/stm(db/da/ib)
144+ to access doublewords. The supported load/store offsets are
145+ -8, -4, and 4, which we try to produce here. */
146+ low = ((val & 0xf) ^ 0x8) - 0x8;
147+ }
148+ else if (GET_MODE_SIZE (mode) < 8)
149+ {
150+ /* NEON element load/stores do not have an offset. */
151+ if (TARGET_NEON_FP16 && mode == HFmode)
152+ return false;
153+
154+ if (TARGET_THUMB2)
155+ {
156+ /* Thumb-2 has an asymmetrical index range of (-256,4096).
157+ Try the wider 12-bit range first, and re-try if the result
158+ is out of range. */
159+ low = SIGN_MAG_LOW_ADDR_BITS (val, 12);
160+ if (low < -255)
161+ low = SIGN_MAG_LOW_ADDR_BITS (val, 8);
162+ }
163+ else
164+ {
165+ if (mode == HImode || mode == HFmode)
166+ {
167+ if (arm_arch4)
168+ low = SIGN_MAG_LOW_ADDR_BITS (val, 8);
169+ else
170+ {
171+ /* The storehi/movhi_bytes fallbacks can use only
172+ [-4094,+4094] of the full ldrb/strb index range. */
173+ low = SIGN_MAG_LOW_ADDR_BITS (val, 12);
174+ if (low == 4095 || low == -4095)
175+ return false;
176+ }
177+ }
178+ else
179+ low = SIGN_MAG_LOW_ADDR_BITS (val, 12);
180+ }
181+ }
182 else
183 return false;
184
185
186=== modified file 'gcc/config/arm/arm.md'
187--- gcc/config/arm/arm.md 2011-06-02 15:58:33 +0000
188+++ gcc/config/arm/arm.md 2011-06-09 09:02:19 +0000
189@@ -6245,7 +6245,7 @@
190 [(match_operand:DF 0 "arm_reload_memory_operand" "=o")
191 (match_operand:DF 1 "s_register_operand" "r")
192 (match_operand:SI 2 "s_register_operand" "=&r")]
193- "TARGET_32BIT"
194+ "TARGET_THUMB2"
195 "
196 {
197 enum rtx_code code = GET_CODE (XEXP (operands[0], 0));

Subscribers

People subscribed via source and target branches