diff mbox

[RFA/ARM/4.7] Fix PR54974: Thumb literal pools don't handle PC rounding

Message ID 50E6A9A5.5040706@linaro.org
State New
Headers show

Commit Message

Matthew Gretton-Dann Jan. 4, 2013, 10:06 a.m. UTC 
On 29/11/12 14:42, Matthew Gretton-Dann wrote:
> On 24 November 2012 00:27, Ramana Radhakrishnan
> <ramana.gcc@googlemail.com> wrote:
>> On Wed, Nov 21, 2012 at 7:59 PM, Matthew Gretton-Dann
>> <matthew.gretton-dann@linaro.org> wrote:
> [snip]
>>> The fix is to decrease the pool_range of all insns by 2 when generating
>>> Thumb code.  There is no need to change neg_pool_range values as rounding
>>> down here will reduce the distance of the literal pool.
>>
>> A comment about this fact around thumb2_pool_range would be appropriate.
>>
> [snip]
>>>
>>> Tested arm-none-linux-gnueabi cross, and with the testcase attached to the
>>> PR.  No added testcase in the patch as this code is sensitive to other code
>>> generation and so it is not easy to generate a testcase which will reliably
>>> test this condition.
>>>
>>> OK for trunk, 4.7, and 4.6?
>>
>>
>> Ok for trunk today - please wait a few days before backporting into
>> 4.6 and 4.7 to see what the fallout is like . Watch out for any
>> fallout with the auto-testers.

No fallout has been seen with the auto-testers.

> The attached is what was actually committed as revision 193930
> (original patch + requested comment).

The attached patch is a backport of the trunk patch to 4.7.

Cross tested arm-none-linux-gnueabi with QEMU

OK for 4.7?

Thanks,

Matt

2013-01-04  Matthew Gretton-Dann  <matthew.gretton-dann@linaro.org>

	Backport from mainline.
	2012-11-29  Matthew Gretton-Dann  <matthew.gretton-dann@linaro.org>

	PR target/54974
	* config/arm/arm.md (thumb2_pool_range, pool_range): Add
	comment on Thumb pool ranges.
	(thumb1_extendhisi2): Reduce Thumb pool range.
	(arm_movdi): Likewise.
	(thumb1_movdi_insn): Likewise.
	(thumb1_movsi_insn): Likewise.
	(pic_load_addr_unified): Likewise.
	(pic_load_addr_32bit): Likewise.
	(pic_load_addr_thumb1): Likewise.
	(thumb1_movhf): Likewise.
	(arm_movsf_soft_insn): Likewise.
	(thumb1_movsf_soft_insn): Likewise.
	(movdf_soft_insn): Likewise.
	(thumb1_movdf_soft_insn): Likewise.
	* config/arm/neon.md (*neon_mov<mode>): Likewise.
	(*neon_mov<mode>): Likwise.
	* config/arm/thumb2.md: (*thumb2_movsi_insn): Likewise.
	(*thumb2_movhi_insn): Likewise.
	(*thumb2_extendqisi_v6): Likewise.
	(*thumb2_zero_extendqisi_v6): Likewise.
	(*thumb2_zero_extendqisi2_v6): Likewise.
	* config/arm/vfp.md: (*thumb2_movsi_vfp): Likewise.
	(*movdi_vfp): Likewise.
	(*movdi_vfp_cortexa8): Likewise.
	(*thumb2_movsf_vfp): Likewise.
	(*thumb2_movdf_vfp): Likewise.

Comments

Ye Joey May 10, 2013, 9:59 a.m. UTC | #1 
Ping this for 4.7, as I'm seeing 4.7 users blocked probably by this bug.

- Joey


On Fri, Jan 4, 2013 at 6:06 PM, Matthew Gretton-Dann <
matthew.gretton-dann@linaro.org> wrote:

> On 29/11/12 14:42, Matthew Gretton-Dann wrote:
>
>> On 24 November 2012 00:27, Ramana Radhakrishnan
>> <ramana.gcc@googlemail.com> wrote:
>>
>>> On Wed, Nov 21, 2012 at 7:59 PM, Matthew Gretton-Dann
>>> <matthew.gretton-dann@linaro.**org <matthew.gretton-dann@linaro.org>>
>>> wrote:
>>>
>> [snip]
>>
>>> The fix is to decrease the pool_range of all insns by 2 when generating
>>>> Thumb code.  There is no need to change neg_pool_range values as
>>>> rounding
>>>> down here will reduce the distance of the literal pool.
>>>>
>>>
>>> A comment about this fact around thumb2_pool_range would be appropriate.
>>>
>>>  [snip]
>>
>>>
>>>> Tested arm-none-linux-gnueabi cross, and with the testcase attached to
>>>> the
>>>> PR.  No added testcase in the patch as this code is sensitive to other
>>>> code
>>>> generation and so it is not easy to generate a testcase which will
>>>> reliably
>>>> test this condition.
>>>>
>>>> OK for trunk, 4.7, and 4.6?
>>>>
>>>
>>>
>>> Ok for trunk today - please wait a few days before backporting into
>>> 4.6 and 4.7 to see what the fallout is like . Watch out for any
>>> fallout with the auto-testers.
>>>
>>
> No fallout has been seen with the auto-testers.
>
>
>  The attached is what was actually committed as revision 193930
>> (original patch + requested comment).
>>
>
> The attached patch is a backport of the trunk patch to 4.7.
>
> Cross tested arm-none-linux-gnueabi with QEMU
>
> OK for 4.7?
>
> Thanks,
>
> Matt
>
> 2013-01-04  Matthew Gretton-Dann  <matthew.gretton-dann@linaro.**org<matthew.gretton-dann@linaro.org>
> >
>
>         Backport from mainline.
>         2012-11-29  Matthew Gretton-Dann  <matthew.gretton-dann@linaro.**
> org <matthew.gretton-dann@linaro.org>>
>
>         PR target/54974
>         * config/arm/arm.md (thumb2_pool_range, pool_range): Add
>         comment on Thumb pool ranges.
>         (thumb1_extendhisi2): Reduce Thumb pool range.
>
>         (arm_movdi): Likewise.
>         (thumb1_movdi_insn): Likewise.
>         (thumb1_movsi_insn): Likewise.
>         (pic_load_addr_unified): Likewise.
>         (pic_load_addr_32bit): Likewise.
>         (pic_load_addr_thumb1): Likewise.
>         (thumb1_movhf): Likewise.
>         (arm_movsf_soft_insn): Likewise.
>         (thumb1_movsf_soft_insn): Likewise.
>         (movdf_soft_insn): Likewise.
>         (thumb1_movdf_soft_insn): Likewise.
>         * config/arm/neon.md (*neon_mov<mode>): Likewise.
>         (*neon_mov<mode>): Likwise.
>         * config/arm/thumb2.md: (*thumb2_movsi_insn): Likewise.
>         (*thumb2_movhi_insn): Likewise.
>         (*thumb2_extendqisi_v6): Likewise.
>         (*thumb2_zero_extendqisi_v6): Likewise.
>         (*thumb2_zero_extendqisi2_v6): Likewise.
>         * config/arm/vfp.md: (*thumb2_movsi_vfp): Likewise.
>         (*movdi_vfp): Likewise.
>         (*movdi_vfp_cortexa8): Likewise.
>         (*thumb2_movsf_vfp): Likewise.
>         (*thumb2_movdf_vfp): Likewise.
>
> --
> Matthew Gretton-Dann
> Toolchain Working Group, Linaro
>
Ramana Radhakrishnan May 10, 2013, 10:13 a.m. UTC | #2 
> OK for 4.7?
>


Ok - I did say ok if no fallout in my original review :)


regards
Ramana
> Thanks,
>
> Matt
>
> 2013-01-04  Matthew Gretton-Dann  <matthew.gretton-dann@linaro.org>
>
> 	Backport from mainline.
> 	2012-11-29  Matthew Gretton-Dann  <matthew.gretton-dann@linaro.org>
>
> 	PR target/54974
> 	* config/arm/arm.md (thumb2_pool_range, pool_range): Add
> 	comment on Thumb pool ranges.
> 	(thumb1_extendhisi2): Reduce Thumb pool range.
> 	(arm_movdi): Likewise.
> 	(thumb1_movdi_insn): Likewise.
> 	(thumb1_movsi_insn): Likewise.
> 	(pic_load_addr_unified): Likewise.
> 	(pic_load_addr_32bit): Likewise.
> 	(pic_load_addr_thumb1): Likewise.
> 	(thumb1_movhf): Likewise.
> 	(arm_movsf_soft_insn): Likewise.
> 	(thumb1_movsf_soft_insn): Likewise.
> 	(movdf_soft_insn): Likewise.
> 	(thumb1_movdf_soft_insn): Likewise.
> 	* config/arm/neon.md (*neon_mov<mode>): Likewise.
> 	(*neon_mov<mode>): Likwise.
> 	* config/arm/thumb2.md: (*thumb2_movsi_insn): Likewise.
> 	(*thumb2_movhi_insn): Likewise.
> 	(*thumb2_extendqisi_v6): Likewise.
> 	(*thumb2_zero_extendqisi_v6): Likewise.
> 	(*thumb2_zero_extendqisi2_v6): Likewise.
> 	* config/arm/vfp.md: (*thumb2_movsi_vfp): Likewise.
> 	(*movdi_vfp): Likewise.
> 	(*movdi_vfp_cortexa8): Likewise.
> 	(*thumb2_movsf_vfp): Likewise.
> 	(*thumb2_movdf_vfp): Likewise.
>
Joey Ye May 10, 2013, 3:27 p.m. UTC | #3 
> -----Original Message-----
> From: Ramana Radhakrishnan
> Sent: Friday, May 10, 2013 18:13
> To: Matthew Gretton-Dann
> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw; doko@canonical.com; Patch
> Tracking; Richard Biener; Joey Ye
> Subject: Re: [RFA/ARM/4.7] Fix PR54974: Thumb literal pools don't handle
PC
> rounding
> 
> 
> > OK for 4.7?
> >
> 
> 
> Ok - I did say ok if no fallout in my original review :)
Tested with Qemu Cortex-M3. No regression.

Committed to 4.7.

- Joey
diff mbox

Patch

Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 194852)
+++ gcc/config/arm/arm.md	(working copy)
@@ -256,6 +256,9 @@  (define_attr "insn_enabled" "no,yes"
 ; POOL_RANGE is how far away from a constant pool entry that this insn
 ; can be placed.  If the distance is zero, then this insn will never
 ; reference the pool.
+; Note that for Thumb constant pools the PC value is rounded down to the
+; nearest multiple of four.  Therefore, THUMB2_POOL_RANGE (and POOL_RANGE for
+; Thumb insns) should be set to <max_range> - 2.
 ; NEG_POOL_RANGE is nonzero for insns that can reference a constant pool entry
 ; before its address.  It is set to <max_range> - (8 + <data_size>).
 (define_attr "arm_pool_range" "" (const_int 0))
@@ -4833,7 +4836,7 @@  (define_insn "thumb1_extendhisi2"
 					(const_int 2) (const_int 4))
 			  (const_int 4)])
    (set_attr "type" "alu_shift,load_byte")
-   (set_attr "pool_range" "*,1020")]
+   (set_attr "pool_range" "*,1018")]
 )
 
 ;; This pattern will only be used when ldsh is not available
@@ -5239,7 +5242,7 @@  (define_insn "*arm_movdi"
    (set_attr "type" "*,*,*,load2,store2")
    (set_attr "arm_pool_range" "*,*,*,1020,*")
    (set_attr "arm_neg_pool_range" "*,*,*,1004,*")
-   (set_attr "thumb2_pool_range" "*,*,*,4096,*")
+   (set_attr "thumb2_pool_range" "*,*,*,4094,*")
    (set_attr "thumb2_neg_pool_range" "*,*,*,0,*")]
 )
 
@@ -5379,7 +5382,7 @@  (define_insn "*thumb1_movdi_insn"
   [(set_attr "length" "4,4,6,2,2,6,4,4")
    (set_attr "type" "*,*,*,load2,store2,load2,store2,*")
    (set_attr "insn" "*,mov,*,*,*,*,*,mov")
-   (set_attr "pool_range" "*,*,*,*,*,1020,*,*")]
+   (set_attr "pool_range" "*,*,*,*,*,1018,*,*")]
 )
 
 (define_expand "movsi"
@@ -5539,7 +5542,7 @@  (define_insn "*thumb1_movsi_insn"
    mov\\t%0, %1"
   [(set_attr "length" "2,2,4,4,2,2,2,2,2")
    (set_attr "type" "*,*,*,*,load1,store1,load1,store1,*")
-   (set_attr "pool_range" "*,*,*,*,*,*,1020,*,*")
+   (set_attr "pool_range" "*,*,*,*,*,*,1018,*,*")
    (set_attr "conds" "set,clob,*,*,nocond,nocond,nocond,nocond,nocond")])
 
 (define_split 
@@ -5632,7 +5635,7 @@  (define_insn_and_split "pic_load_addr_un
        		     		 (match_dup 2)] UNSPEC_PIC_BASE))]
  "operands[3] = TARGET_THUMB ? GEN_INT (4) : GEN_INT (8);"
  [(set_attr "type" "load1,load1,load1")
-  (set_attr "pool_range" "4096,4096,1024")
+  (set_attr "pool_range" "4096,4094,1022")
   (set_attr "neg_pool_range" "4084,0,0")
   (set_attr "arch"  "a,t2,t1")    
   (set_attr "length" "8,6,4")]
@@ -5648,7 +5651,10 @@  (define_insn "pic_load_addr_32bit"
   "TARGET_32BIT && flag_pic"
   "ldr%?\\t%0, %1"
   [(set_attr "type" "load1")
-   (set_attr "pool_range" "4096")
+   (set (attr "pool_range")
+	(if_then_else (eq_attr "is_thumb" "no")
+		      (const_int 4096)
+		      (const_int 4094)))
    (set (attr "neg_pool_range")
 	(if_then_else (eq_attr "is_thumb" "no")
 		      (const_int 4084)
@@ -5661,7 +5667,7 @@  (define_insn "pic_load_addr_thumb1"
   "TARGET_THUMB1 && flag_pic"
   "ldr\\t%0, %1"
   [(set_attr "type" "load1")
-   (set (attr "pool_range") (const_int 1024))]
+   (set (attr "pool_range") (const_int 1018))]
 )
 
 (define_insn "pic_add_dot_plus_four"
@@ -6456,7 +6462,7 @@  (define_insn "*thumb1_movhf"
   [(set_attr "length" "2")
    (set_attr "type" "*,load1,store1,*,*")
    (set_attr "insn" "mov,*,*,mov,mov")
-   (set_attr "pool_range" "*,1020,*,*,*")
+   (set_attr "pool_range" "*,1018,*,*,*")
    (set_attr "conds" "clob,nocond,nocond,nocond,nocond")])
 
 (define_expand "movsf"
@@ -6511,7 +6517,8 @@  (define_insn "*arm_movsf_soft_insn"
   [(set_attr "predicable" "yes")
    (set_attr "type" "*,load1,store1")
    (set_attr "insn" "mov,*,*")
-   (set_attr "pool_range" "*,4096,*")
+   (set_attr "arm_pool_range" "*,4096,*")
+   (set_attr "thumb2_pool_range" "*,4094,*")
    (set_attr "arm_neg_pool_range" "*,4084,*")
    (set_attr "thumb2_neg_pool_range" "*,0,*")]
 )
@@ -6533,7 +6540,7 @@  (define_insn "*thumb1_movsf_insn"
    mov\\t%0, %1"
   [(set_attr "length" "2")
    (set_attr "type" "*,load1,store1,load1,store1,*,*")
-   (set_attr "pool_range" "*,*,*,1020,*,*,*")
+   (set_attr "pool_range" "*,*,*,1018,*,*,*")
    (set_attr "insn" "*,*,*,*,*,mov,mov")
    (set_attr "conds" "clob,nocond,nocond,nocond,nocond,nocond,nocond")]
 )
@@ -6622,7 +6629,8 @@  (define_insn "*movdf_soft_insn"
   "
   [(set_attr "length" "8,12,16,8,8")
    (set_attr "type" "*,*,*,load2,store2")
-   (set_attr "pool_range" "*,*,*,1020,*")
+   (set_attr "arm_pool_range" "*,*,*,1020,*")
+   (set_attr "thumb2_pool_range" "*,*,*,1018,*")
    (set_attr "arm_neg_pool_range" "*,*,*,1004,*")
    (set_attr "thumb2_neg_pool_range" "*,*,*,0,*")]
 )
@@ -6665,7 +6673,7 @@  (define_insn "*thumb_movdf_insn"
   [(set_attr "length" "4,2,2,6,4,4")
    (set_attr "type" "*,load2,store2,load2,store2,*")
    (set_attr "insn" "*,*,*,*,*,mov")
-   (set_attr "pool_range" "*,*,*,1020,*,*")]
+   (set_attr "pool_range" "*,*,*,1018,*,*")]
 )
 
 (define_expand "movxf"
Index: gcc/config/arm/neon.md
===================================================================
--- gcc/config/arm/neon.md	(revision 194852)
+++ gcc/config/arm/neon.md	(working copy)
@@ -201,7 +201,8 @@  (define_insn "*neon_mov<mode>"
   (set_attr "type" "*,f_stored,*,f_loadd,*,*,alu,load2,store2")
   (set_attr "insn" "*,*,*,*,*,*,mov,*,*")
   (set_attr "length" "4,4,4,4,4,4,8,8,8")
-  (set_attr "pool_range"     "*,*,*,1020,*,*,*,1020,*")
+  (set_attr "arm_pool_range"     "*,*,*,1020,*,*,*,1020,*")
+  (set_attr "thumb2_pool_range"     "*,*,*,1018,*,*,*,1018,*")
   (set_attr "neg_pool_range" "*,*,*,1004,*,*,*,1004,*")])
 
 (define_insn "*neon_mov<mode>"
@@ -246,7 +247,8 @@  (define_insn "*neon_mov<mode>"
    (set_attr "type" "*,*,*,*,*,*,alu,load4,store4")
    (set_attr "insn" "*,*,*,*,*,*,mov,*,*")
    (set_attr "length" "4,8,4,8,8,8,16,8,16")
-   (set_attr "pool_range" "*,*,*,1020,*,*,*,1020,*")
+   (set_attr "arm_pool_range" "*,*,*,1020,*,*,*,1020,*")
+   (set_attr "thumb2_pool_range" "*,*,*,1018,*,*,*,1018,*")
    (set_attr "neg_pool_range" "*,*,*,996,*,*,*,996,*")])
 
 (define_expand "movti"
Index: gcc/config/arm/vfp.md
===================================================================
--- gcc/config/arm/vfp.md	(revision 194852)
+++ gcc/config/arm/vfp.md	(working copy)
@@ -126,7 +126,7 @@  (define_insn "*thumb2_movsi_vfp"
   [(set_attr "predicable" "yes")
    (set_attr "type" "*,*,*,*,load1,load1,store1,store1,r_2_f,f_2_r,fcpys,f_loads,f_stores")
    (set_attr "insn" "mov,mov,mvn,mov,*,*,*,*,*,*,*,*,*")
-   (set_attr "pool_range"     "*,*,*,*,1020,4096,*,*,*,*,*,1020,*")
+   (set_attr "pool_range"     "*,*,*,*,1018,4094,*,*,*,*,*,1018,*")
    (set_attr "neg_pool_range" "*,*,*,*,   0,   0,*,*,*,*,*,1008,*")]
 )
 
@@ -177,7 +177,8 @@  (define_insn "*movdi_vfp"
                                  (const_int 8)
                                  (const_int 4))]
                               (const_int 4)))
-   (set_attr "pool_range"     "*,*,*,*,1020,4096,*,*,*,*,1020,*")
+   (set_attr "arm_pool_range"     "*,*,*,*,1020,4096,*,*,*,*,1020,*")
+   (set_attr "thumb2_pool_range"     "*,*,*,*,1018,4094,*,*,*,*,1018,*")
    (set_attr "neg_pool_range" "*,*,*,*,1004,0,*,*,*,*,1004,*")
    (set_attr "arch"           "t2,any,any,any,a,t2,any,any,any,any,any,any")]
 )
@@ -222,7 +223,8 @@  (define_insn "*movdi_vfp_cortexa8"
                                  * 4")]
                               (const_int 4)))
    (set_attr "predicable"    "yes")
-   (set_attr "pool_range"     "*,*,*,*,1020,4096,*,*,*,*,1020,*")
+   (set_attr "arm_pool_range"     "*,*,*,*,1018,4094,*,*,*,*,1018,*")
+   (set_attr "thumb2_pool_range"     "*,*,*,*,1018,4094,*,*,*,*,1018,*")
    (set_attr "neg_pool_range" "*,*,*,*,1004,0,*,*,*,*,1004,*")
    (set (attr "ce_count") 
 	(symbol_ref "get_attr_length (insn) / 4"))
@@ -409,7 +411,7 @@  (define_insn "*thumb2_movsf_vfp"
    (set_attr "type"
      "r_2_f,f_2_r,fconsts,f_loads,f_stores,load1,store1,fcpys,*")
    (set_attr "insn" "*,*,*,*,*,*,*,*,mov")
-   (set_attr "pool_range" "*,*,*,1020,*,4092,*,*,*")
+   (set_attr "pool_range" "*,*,*,1018,*,4090,*,*,*")
    (set_attr "neg_pool_range" "*,*,*,1008,*,0,*,*,*")]
 )
 
@@ -501,7 +503,7 @@  (define_insn "*thumb2_movdf_vfp"
 				 (const_int 8)
 				 (const_int 4))]
 			      (const_int 4)))
-   (set_attr "pool_range" "*,*,*,1020,*,4096,*,*,*")
+   (set_attr "pool_range" "*,*,*,1018,*,4094,*,*,*")
    (set_attr "neg_pool_range" "*,*,*,1008,*,0,*,*,*")]
 )
 
Index: gcc/config/arm/thumb2.md
===================================================================
--- gcc/config/arm/thumb2.md	(revision 194852)
+++ gcc/config/arm/thumb2.md	(working copy)
@@ -182,7 +182,7 @@  (define_insn "*thumb2_movsi_insn"
    str%?\\t%1, %0"
   [(set_attr "type" "*,*,*,*,load1,load1,store1,store1")
    (set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,*,1020,4096,*,*")
+   (set_attr "pool_range" "*,*,*,*,1018,4094,*,*")
    (set_attr "neg_pool_range" "*,*,*,*,0,0,*,*")]
 )
 
@@ -217,7 +217,7 @@  (define_insn "*thumb2_movhi_insn"
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "type" "*,*,store1,load1")
    (set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,4096")
+   (set_attr "pool_range" "*,*,*,4094")
    (set_attr "neg_pool_range" "*,*,*,250")]
 )
 
@@ -570,7 +570,7 @@  (define_insn "*thumb2_extendqisi_v6"
    ldr%(sb%)\\t%0, %1"
   [(set_attr "type" "alu_shift,load_byte")
    (set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,4096")
+   (set_attr "pool_range" "*,4094")
    (set_attr "neg_pool_range" "*,250")]
 )
 
@@ -583,7 +583,7 @@  (define_insn "*thumb2_zero_extendhisi2_v
    ldr%(h%)\\t%0, %1"
   [(set_attr "type" "alu_shift,load_byte")
    (set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,4096")
+   (set_attr "pool_range" "*,4094")
    (set_attr "neg_pool_range" "*,250")]
 )
 
@@ -596,7 +596,7 @@  (define_insn "thumb2_zero_extendqisi2_v6
    ldr%(b%)\\t%0, %1\\t%@ zero_extendqisi2"
   [(set_attr "type" "alu_shift,load_byte")
    (set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,4096")
+   (set_attr "pool_range" "*,4094")
    (set_attr "neg_pool_range" "*,250")]
 )