diff mbox

ARM patch: Add "insn" attribute to more patterns.

Message ID 4C6BB412.6080601@codesourcery.com
State New
Headers show

Commit Message

Bernd Schmidt Aug. 18, 2010, 10:21 a.m. UTC 
I discovered by accident that the Cortex-A8 isn't working right in all
cases.  It expects attribute "insn" to have a value of "mov" for move
instructions, but this attribute isn't set in all places where it would
be necessary.

I found a patch in our tree which corrects this.  Tested on QEMU,
arm-linux, Thumb-2, Thumb-1 and ARM.  Ok?


Bernd
* config/arm/vfp.md (arm_movsi_vfp, thumb2_movsi_vfp, movsf_vfp,
	thumb2_movsf_vfp): Set attribute "insn".
	* config/arm/arm.md (arm_ashrdi3_1bit, arm_lshrdi3_1bit, not_shiftsi,
	not_shiftsi_compare0, not_shiftsi_compare0_scratch, arm_one_cmplsi2,
	thumb1_one_cmplsi2, notsi_compare0, notsi_compare0_scratch,
	arm_zero_extendsidi2, arm_extendsidi2, thumb1_movdi_insn,
	arm_movsi_insn, movhi_insn_arch4, movhi_bytes, arm_movqi_insn,
	thumb1_movqi_insn arm32_movhf, thumb1_movhf, arm_movsf_soft_insn,
	thumb1_movsf_insn, thumb_movdf_insn, mov_scc, mov_negscc, mov_notscc,
	movsicc_insn, movsfcc_soft_insn, and_scc, cond_move, if_move_not,
	if_not_move, if_shift_move, if_move_shift, if_shift_shift,
	if_not_arith, if_arith_not, cond_move_not): Likewise.

Comments

Ramana Radhakrishnan Aug. 19, 2010, 10:48 a.m. UTC | #1 
Hi Bernd,

	The *di patterns in a few cases actually map to 2 instructions that
have a dependency on the flag bit. Hence this should get executed in 2
cycles. Thus if we are marking them as "mov" we assume that they finish
executing in 1 cycle. I don't think we worry about this at all. 

Do we want to create another attribute type here to use all ALU units in
2 consecutive cycles ? This could be a follow up patch based on
experimentation for later.

Other specific issues are inline below.

>  
> @@ -9486,6 +9513,7 @@ (define_insn "*if_not_move"
>     mov%D4\\t%0, %1\;mvn%d4\\t%0, %2
>     mvn%D4\\t%0, #%B1\;mvn%d4\\t%0, %2"
>    [(set_attr "conds" "use")
> +   (set_attr "insn" "mvn")
>     (set_attr "length" "4,8,8")]
>  )

Should this be of "type" "alu_shift_reg / alu_shift" similar to the bits
for *if_shift_move below ? 

>  
> @@ -9523,6 +9551,7 @@ (define_insn "*if_shift_move"
>    [(set_attr "conds" "use")
>     (set_attr "shift" "2")
>     (set_attr "length" "4,8,8")
> +   (set_attr "insn" "mov")
>     (set (attr "type") (if_then_else (match_operand 3
> "const_int_operand" "")
>                       (const_string "alu_shift")
>                       (const_string "alu_shift_reg")))]
> @@ -9562,6 +9591,7 @@ (define_insn "*if_move_shift"
>    [(set_attr "conds" "use")
>     (set_attr "shift" "2")
>     (set_attr "length" "4,8,8")
> +   (set_attr "insn" "mov")
>     (set (attr "type") (if_then_else (match_operand 3
> "const_int_operand" "")
>                       (const_string "alu_shift")
>                       (const_string "alu_shift_reg")))]
> @@ -9602,6 +9632,7 @@ (define_insn "*if_shift_shift"
>    [(set_attr "conds" "use")
>     (set_attr "shift" "1")
>     (set_attr "length" "8")
> +   (set_attr "insn" "mov")
>     (set (attr "type") (if_then_else
>                         (and (match_operand 2 "const_int_operand" "")
>                               (match_operand 4 "const_int_operand"
> ""))


> @@ -9638,6 +9669,7 @@ (define_insn "*if_not_arith"
>    "TARGET_ARM"
>    "mvn%d5\\t%0, %1\;%I6%D5\\t%0, %2, %3"
>    [(set_attr "conds" "use")
> +   (set_attr "insn" "mvn")
>     (set_attr "length" "8")]
>  )

Should this also have the "type" set to "alu_shift_reg" because there is
also a shift operation involved here ? 

>  
> @@ -9670,6 +9702,7 @@ (define_insn "*if_arith_not"
>    "TARGET_ARM"
>    "mvn%D5\\t%0, %1\;%I6%d5\\t%0, %2, %3"
>    [(set_attr "conds" "use")
> +   (set_attr "insn" "mvn")
>     (set_attr "length" "8")]
>  )

And here ?


Ramana

--
Richard Earnshaw Aug. 24, 2010, 9 a.m. UTC | #2 
On Wed, 2010-08-18 at 12:21 +0200, Bernd Schmidt wrote:
> I discovered by accident that the Cortex-A8 isn't working right in all
> cases.  It expects attribute "insn" to have a value of "mov" for move
> instructions, but this attribute isn't set in all places where it would
> be necessary.
> 
> I found a patch in our tree which corrects this.  Tested on QEMU,
> arm-linux, Thumb-2, Thumb-1 and ARM.  Ok?
> 
> 
> Bernd

I think Ramana raises some valid questions, but I don't think they mean
that this patch is not a positive step forward.

However, one minor niggle:

@@ -6240,8 +6259,8 @@ (define_insn "*thumb_movdf_insn"
   "
   [(set_attr "length" "4,2,2,6,4,4")
    (set_attr "type" "*,load2,store2,load2,store2,*")
-   (set_attr "pool_range" "*,*,*,1020,*,*")]
-)
+   (set_attr "insn" "*,*,*,*,*,mov")
+   (set_attr "pool_range" "*,*,*,1020,*,*")])
 

Your patch is inconsistent in how you've terminated patterns.  The
prevailing style in the ARM machine description is for the bracket that
closes a pattern to be on a line of its own (starting in the first
column).  Please stick to the prevailing style rather than gratuitously
changing things.

OK once that's fixed.

R.
Bernd Schmidt Aug. 27, 2010, 10:15 p.m. UTC | #3 
On 08/24/2010 11:00 AM, Richard Earnshaw wrote:

> Your patch is inconsistent in how you've terminated patterns.  The
> prevailing style in the ARM machine description is for the bracket that
> closes a pattern to be on a line of its own (starting in the first
> column).  Please stick to the prevailing style rather than gratuitously
> changing things.
> 
> OK once that's fixed.

Committed with this change.  I've made similar changes before out of
habit, since this style is inconsistent with how RTL is usually written
and it wastes vertical space.  Can we change it in config/arm?


Bernd
diff mbox

Patch

Index: config/arm/vfp.md
===================================================================
--- config/arm/vfp.md	(revision 163280)
+++ config/arm/vfp.md	(working copy)
@@ -82,6 +82,7 @@  (define_insn "*arm_movsi_vfp"
   "
   [(set_attr "predicable" "yes")
    (set_attr "type" "*,*,*,*,load1,store1,r_2_f,f_2_r,fcpys,f_loads,f_stores")
+   (set_attr "insn" "mov,mov,mvn,mov,*,*,*,*,*,*,*")
    (set_attr "pool_range"     "*,*,*,*,4096,*,*,*,*,1020,*")
    (set_attr "neg_pool_range" "*,*,*,*,4084,*,*,*,*,1008,*")]
 )
@@ -123,6 +124,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_load,f_store")
+   (set_attr "insn" "mov,mov,mvn,mov,*,*,*,*,*,*,*,*,*")
    (set_attr "pool_range"     "*,*,*,*,1020,4096,*,*,*,*,*,1020,*")
    (set_attr "neg_pool_range" "*,*,*,*,   0,   0,*,*,*,*,*,1008,*")]
 )
@@ -352,6 +354,7 @@  (define_insn "*movsf_vfp"
   [(set_attr "predicable" "yes")
    (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,*,4096,*,*,*")
    (set_attr "neg_pool_range" "*,*,*,1008,*,4080,*,*,*")]
 )
@@ -388,6 +391,7 @@  (define_insn "*thumb2_movsf_vfp"
   [(set_attr "predicable" "yes")
    (set_attr "type"
      "r_2_f,f_2_r,fconsts,f_load,f_store,load1,store1,fcpys,*")
+   (set_attr "insn" "*,*,*,*,*,*,*,*,mov")
    (set_attr "pool_range" "*,*,*,1020,*,4092,*,*,*")
    (set_attr "neg_pool_range" "*,*,*,1008,*,0,*,*,*")]
 )
Index: config/arm/arm.md
===================================================================
--- config/arm/arm.md	(revision 163281)
+++ config/arm/arm.md	(working copy)
@@ -3366,6 +3366,7 @@  (define_insn "arm_ashrdi3_1bit"
   "TARGET_32BIT"
   "movs\\t%R0, %R1, asr #1\;mov\\t%Q0, %Q1, rrx"
   [(set_attr "conds" "clob")
+   (set_attr "insn" "mov")
    (set_attr "length" "8")]
 )
 
@@ -3422,6 +3423,7 @@  (define_insn "arm_lshrdi3_1bit"
   "TARGET_32BIT"
   "movs\\t%R0, %R1, lsr #1\;mov\\t%Q0, %Q1, rrx"
   [(set_attr "conds" "clob")
+   (set_attr "insn" "mov")
    (set_attr "length" "8")]
 )
 
@@ -3548,6 +3550,7 @@  (define_insn "*not_shiftsi"
   "mvn%?\\t%0, %1%S3"
   [(set_attr "predicable" "yes")
    (set_attr "shift" "1")
+   (set_attr "insn" "mvn")
    (set_attr "arch" "32,a")
    (set_attr "type" "alu_shift,alu_shift_reg")])
 
@@ -3564,6 +3567,7 @@  (define_insn "*not_shiftsi_compare0"
   "mvn%.\\t%0, %1%S3"
   [(set_attr "conds" "set")
    (set_attr "shift" "1")
+   (set_attr "insn" "mvn")
    (set_attr "arch" "32,a")
    (set_attr "type" "alu_shift,alu_shift_reg")])
 
@@ -3579,6 +3583,7 @@  (define_insn "*not_shiftsi_compare0_scra
   "mvn%.\\t%0, %1%S3"
   [(set_attr "conds" "set")
    (set_attr "shift" "1")
+   (set_attr "insn" "mvn")
    (set_attr "arch" "32,a")
    (set_attr "type" "alu_shift,alu_shift_reg")])
 
@@ -3838,7 +3843,8 @@  (define_insn "*arm_one_cmplsi2"
 	(not:SI (match_operand:SI 1 "s_register_operand"  "r")))]
   "TARGET_32BIT"
   "mvn%?\\t%0, %1"
-  [(set_attr "predicable" "yes")]
+  [(set_attr "predicable" "yes")
+   (set_attr "insn" "mvn")]
 )
 
 (define_insn "*thumb1_one_cmplsi2"
@@ -3846,7 +3852,8 @@  (define_insn "*thumb1_one_cmplsi2"
 	(not:SI (match_operand:SI 1 "register_operand"  "l")))]
   "TARGET_THUMB1"
   "mvn\\t%0, %1"
-  [(set_attr "length" "2")]
+  [(set_attr "length" "2")
+   (set_attr "insn" "mvn")]
 )
 
 (define_insn "*notsi_compare0"
@@ -3857,7 +3864,8 @@  (define_insn "*notsi_compare0"
 	(not:SI (match_dup 1)))]
   "TARGET_32BIT"
   "mvn%.\\t%0, %1"
-  [(set_attr "conds" "set")]
+  [(set_attr "conds" "set")
+   (set_attr "insn" "mvn")]
 )
 
 (define_insn "*notsi_compare0_scratch"
@@ -3867,7 +3875,8 @@  (define_insn "*notsi_compare0_scratch"
    (clobber (match_scratch:SI 0 "=r"))]
   "TARGET_32BIT"
   "mvn%.\\t%0, %1"
-  [(set_attr "conds" "set")]
+  [(set_attr "conds" "set")
+   (set_attr "insn" "mvn")]
 )
 
 ;; Fixed <--> Floating conversion insns
@@ -4024,6 +4033,7 @@  (define_insn "*arm_zero_extendsidi2"
     return \"mov%?\\t%R0, #0\";
   "
   [(set_attr "length" "8")
+   (set_attr "insn" "mov")
    (set_attr "predicable" "yes")]
 )
 
@@ -4067,6 +4077,7 @@  (define_insn "*arm_extendsidi2"
   "
   [(set_attr "length" "8")
    (set_attr "shift" "1")
+   (set_attr "insn" "mov")
    (set_attr "predicable" "yes")]
 )
 
@@ -4991,6 +5002,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,*,*")]
 )
 
@@ -5113,6 +5125,7 @@  (define_insn "*arm_movsi_insn"
    ldr%?\\t%0, %1
    str%?\\t%1, %0"
   [(set_attr "type" "*,*,*,*,load1,store1")
+   (set_attr "insn" "mov,mov,mvn,mov,*,*")
    (set_attr "predicable" "yes")
    (set_attr "pool_range" "*,*,*,*,4096,*")
    (set_attr "neg_pool_range" "*,*,*,*,4084,*")]
@@ -5768,6 +5781,7 @@  (define_insn "*movhi_insn_arch4"
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "type" "*,*,store1,load1")
    (set_attr "predicable" "yes")
+   (set_attr "insn" "mov,mvn,*,*")
    (set_attr "pool_range" "*,*,*,256")
    (set_attr "neg_pool_range" "*,*,*,244")]
 )
@@ -5779,7 +5793,8 @@  (define_insn "*movhi_bytes"
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi"
-  [(set_attr "predicable" "yes")]
+  [(set_attr "predicable" "yes")
+   (set_attr "insn" "mov,mvn")]
 )
 
 (define_expand "thumb_movhi_clobber"
@@ -5915,6 +5930,7 @@  (define_insn "*arm_movqi_insn"
    ldr%(b%)\\t%0, %1
    str%(b%)\\t%1, %0"
   [(set_attr "type" "*,*,load1,store1")
+   (set_attr "insn" "mov,mvn,*,*")
    (set_attr "predicable" "yes")]
 )
 
@@ -5933,6 +5949,7 @@  (define_insn "*thumb1_movqi_insn"
    mov\\t%0, %1"
   [(set_attr "length" "2")
    (set_attr "type" "*,load1,store1,*,*,*")
+   (set_attr "insn" "*,*,*,mov,mov,mov")
    (set_attr "pool_range" "*,32,*,*,*,*")
    (set_attr "conds" "clob,nocond,nocond,nocond,nocond,clob")])
 
@@ -5998,9 +6015,9 @@  (define_insn "*arm32_movhf"
   "
   [(set_attr "conds" "unconditional")
    (set_attr "type" "load1,store1,*,*")
+   (set_attr "insn" "*,*,mov,mov")
    (set_attr "length" "4,4,4,8")
-   (set_attr "predicable" "yes")
-   ]
+   (set_attr "predicable" "yes")]
 )
 
 (define_insn "*thumb1_movhf"
@@ -6034,6 +6051,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 "conds" "clob,nocond,nocond,nocond,nocond")])
 
@@ -6088,6 +6106,7 @@  (define_insn "*arm_movsf_soft_insn"
    str%?\\t%1, %0\\t%@ float"
   [(set_attr "predicable" "yes")
    (set_attr "type" "*,load1,store1")
+   (set_attr "insn" "mov,*,*")
    (set_attr "pool_range" "*,4096,*")
    (set_attr "arm_neg_pool_range" "*,4084,*")
    (set_attr "thumb2_neg_pool_range" "*,0,*")]
@@ -6111,8 +6130,8 @@  (define_insn "*thumb1_movsf_insn"
   [(set_attr "length" "2")
    (set_attr "type" "*,load1,store1,load1,store1,*,*")
    (set_attr "pool_range" "*,*,*,1020,*,*,*")
-   (set_attr "conds" "clob,nocond,nocond,nocond,nocond,nocond,nocond")]
-)
+   (set_attr "insn" "*,*,*,*,*,mov,mov")
+   (set_attr "conds" "clob,nocond,nocond,nocond,nocond,nocond,nocond")])
 
 (define_expand "movdf"
   [(set (match_operand:DF 0 "general_operand" "")
@@ -6240,8 +6259,8 @@  (define_insn "*thumb_movdf_insn"
   "
   [(set_attr "length" "4,2,2,6,4,4")
    (set_attr "type" "*,load2,store2,load2,store2,*")
-   (set_attr "pool_range" "*,*,*,1020,*,*")]
-)
+   (set_attr "insn" "*,*,*,*,*,mov")
+   (set_attr "pool_range" "*,*,*,1020,*,*")])
 
 (define_expand "movxf"
   [(set (match_operand:XF 0 "general_operand" "")
@@ -7287,6 +7306,7 @@  (define_insn "*mov_scc"
   "TARGET_ARM"
   "mov%D1\\t%0, #0\;mov%d1\\t%0, #1"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mov")
    (set_attr "length" "8")]
 )
 
@@ -7297,6 +7317,7 @@  (define_insn "*mov_negscc"
   "TARGET_ARM"
   "mov%D1\\t%0, #0\;mvn%d1\\t%0, #0"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mov")
    (set_attr "length" "8")]
 )
 
@@ -7307,6 +7328,7 @@  (define_insn "*mov_notscc"
   "TARGET_ARM"
   "mov%D1\\t%0, #0\;mvn%d1\\t%0, #1"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mov")
    (set_attr "length" "8")]
 )
 
@@ -7670,7 +7692,8 @@  (define_insn "*movsicc_insn"
    mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
    mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2"
   [(set_attr "length" "4,4,4,4,8,8,8,8")
-   (set_attr "conds" "use")]
+   (set_attr "conds" "use")
+   (set_attr "insn" "mov,mvn,mov,mvn,mov,mov,mvn,mvn")]
 )
 
 (define_insn "*movsfcc_soft_insn"
@@ -7683,7 +7706,8 @@  (define_insn "*movsfcc_soft_insn"
   "@
    mov%D3\\t%0, %2
    mov%d3\\t%0, %1"
-  [(set_attr "conds" "use")]
+  [(set_attr "conds" "use")
+   (set_attr "insn" "mov")]
 )
 
 
@@ -8585,6 +8609,7 @@  (define_insn "*and_scc"
   "TARGET_ARM"
   "mov%D1\\t%0, #0\;and%d1\\t%0, %2, #1"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mov")
    (set_attr "length" "8")]
 )
 
@@ -8738,6 +8763,7 @@  (define_insn "*cond_move"
     return \"\";
   "
   [(set_attr "conds" "use")
+   (set_attr "insn" "mov")
    (set_attr "length" "4,4,8")]
 )
 
@@ -9454,6 +9480,7 @@  (define_insn "*if_move_not"
    mov%d4\\t%0, %1\;mvn%D4\\t%0, %2
    mvn%d4\\t%0, #%B1\;mvn%D4\\t%0, %2"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mvn")
    (set_attr "length" "4,8,8")]
 )
 
@@ -9486,6 +9513,7 @@  (define_insn "*if_not_move"
    mov%D4\\t%0, %1\;mvn%d4\\t%0, %2
    mvn%D4\\t%0, #%B1\;mvn%d4\\t%0, %2"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mvn")
    (set_attr "length" "4,8,8")]
 )
 
@@ -9523,6 +9551,7 @@  (define_insn "*if_shift_move"
   [(set_attr "conds" "use")
    (set_attr "shift" "2")
    (set_attr "length" "4,8,8")
+   (set_attr "insn" "mov")
    (set (attr "type") (if_then_else (match_operand 3 "const_int_operand" "")
 		      (const_string "alu_shift")
 		      (const_string "alu_shift_reg")))]
@@ -9562,6 +9591,7 @@  (define_insn "*if_move_shift"
   [(set_attr "conds" "use")
    (set_attr "shift" "2")
    (set_attr "length" "4,8,8")
+   (set_attr "insn" "mov")
    (set (attr "type") (if_then_else (match_operand 3 "const_int_operand" "")
 		      (const_string "alu_shift")
 		      (const_string "alu_shift_reg")))]
@@ -9602,6 +9632,7 @@  (define_insn "*if_shift_shift"
   [(set_attr "conds" "use")
    (set_attr "shift" "1")
    (set_attr "length" "8")
+   (set_attr "insn" "mov")
    (set (attr "type") (if_then_else
 		        (and (match_operand 2 "const_int_operand" "")
                              (match_operand 4 "const_int_operand" ""))
@@ -9638,6 +9669,7 @@  (define_insn "*if_not_arith"
   "TARGET_ARM"
   "mvn%d5\\t%0, %1\;%I6%D5\\t%0, %2, %3"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mvn")
    (set_attr "length" "8")]
 )
 
@@ -9670,6 +9702,7 @@  (define_insn "*if_arith_not"
   "TARGET_ARM"
   "mvn%D5\\t%0, %1\;%I6%d5\\t%0, %2, %3"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mvn")
    (set_attr "length" "8")]
 )
 
@@ -10116,6 +10149,7 @@  (define_insn "*cond_move_not"
    mvn%D4\\t%0, %2
    mov%d4\\t%0, %1\;mvn%D4\\t%0, %2"
   [(set_attr "conds" "use")
+   (set_attr "insn" "mvn")
    (set_attr "length" "4,8")]
 )