diff mbox

ARM patch: Reduce code duplication for Thumb2 move patterns

Message ID 4C3516E6.4090706@codesourcery.com
State New
Headers show

Commit Message

Bernd Schmidt July 8, 2010, 12:08 a.m. UTC 
There are many patterns in thumb2.md which are duplicated with only
minor changes from their ARM equivalents.  This patch removes some of
this duplication.

There's one functional change: the ARM movdi pattern disallows the
mem->mem move case while the Thumb2 one does not; I assumed that the ARM
version was more desirable.

Tested with my usual arm-linux/qemu setup.  Ok?  More patches of this
kind coming if this is considered a good idea.


Bernd
* config/arm/thumb2.md (thumb2_movdi, thumb2_movsf_soft_insn,
	thumb2_movdf_soft_insn): Delete patterns.
	* config/arm/arm.md (arm_pool_range, thumb2_pool_range,
	arm_neg_pool_range, thumb2_neg_pool_range): New attributes.
	(pool_range, neg_pool_range): Use them to define defaults.
	(movdi, arm_movsf_soft_insn, arm_movdf_soft_insn): Define them
	and allow for TARGET_32BIT.

Comments

Bernd Schmidt July 16, 2010, 11:06 a.m. UTC | #1 
Reducing pattern duplication in arm.md/thumb2.md:
  http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00624.html


Bernd
Bernd Schmidt July 23, 2010, 10:20 a.m. UTC | #2 
Reducing pattern duplication in arm.md/thumb2.md:
  http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00624.html


Bernd
Richard Earnshaw July 31, 2010, 1:16 p.m. UTC | #3 
On Thu, 2010-07-08 at 01:08 +0100, Bernd Schmidt wrote:
> There are many patterns in thumb2.md which are duplicated with only
> minor changes from their ARM equivalents.  This patch removes some of
> this duplication.
> 
> There's one functional change: the ARM movdi pattern disallows the
> mem->mem move case while the Thumb2 one does not; I assumed that the
> ARM
> version was more desirable.
> 
> Tested with my usual arm-linux/qemu setup.  Ok?  More patches of this
> kind coming if this is considered a good idea.
> 
> 
> Bernd
> 
> 

This is OK.  

We need to watch out, when merging patterns in this way that we don't
increase the length calculations too much (ideally not at all).  It's
safe, but in the long run is likely to lead to sub-optimal branch and
literal pool generation.

R.
Bernd Schmidt July 31, 2010, 1:50 p.m. UTC | #4 
On 07/31/2010 03:16 PM, Richard Earnshaw wrote:
> On Thu, 2010-07-08 at 01:08 +0100, Bernd Schmidt wrote:
>> There are many patterns in thumb2.md which are duplicated with only
>> minor changes from their ARM equivalents.  This patch removes some of
>> this duplication.
>>
>> There's one functional change: the ARM movdi pattern disallows the
>> mem->mem move case while the Thumb2 one does not; I assumed that the
>> ARM
>> version was more desirable.

> This is OK.  

Thanks!

> We need to watch out, when merging patterns in this way that we don't
> increase the length calculations too much (ideally not at all).  It's
> safe, but in the long run is likely to lead to sub-optimal branch and
> literal pool generation.

I think I checked all the patterns - did you see one where the lengths
were different?


Bernd
Richard Earnshaw July 31, 2010, 1:56 p.m. UTC | #5 
On Sat, 2010-07-31 at 15:50 +0200, Bernd Schmidt wrote:
> On 07/31/2010 03:16 PM, Richard Earnshaw wrote:
> > On Thu, 2010-07-08 at 01:08 +0100, Bernd Schmidt wrote:
> >> There are many patterns in thumb2.md which are duplicated with only
> >> minor changes from their ARM equivalents.  This patch removes some of
> >> this duplication.
> >>
> >> There's one functional change: the ARM movdi pattern disallows the
> >> mem->mem move case while the Thumb2 one does not; I assumed that the
> >> ARM
> >> version was more desirable.
> 
> > This is OK.  
> 
> Thanks!
> 
> > We need to watch out, when merging patterns in this way that we don't
> > increase the length calculations too much (ideally not at all).  It's
> > safe, but in the long run is likely to lead to sub-optimal branch and
> > literal pool generation.
> 
> I think I checked all the patterns - did you see one where the lengths
> were different?

Not in this patch, but the implication of your message was that there
might be more patches like this one to come.  I'm happy to see the
merging, but just wanted to point out that keeping the size information
accurate was important.

R.
Bernd Schmidt July 31, 2010, 2:54 p.m. UTC | #6 
On 07/31/2010 03:56 PM, Richard Earnshaw wrote:
> On Sat, 2010-07-31 at 15:50 +0200, Bernd Schmidt wrote:
>> I think I checked all the patterns - did you see one where the lengths
>> were different?
> 
> Not in this patch, but the implication of your message was that there
> might be more patches like this one to come.  I'm happy to see the
> merging, but just wanted to point out that keeping the size information
> accurate was important.

Ok.

ISTR that Ramana mentioned at some point that he was working on removing
the cond-exec state machine.  I think this would make some other merges
easier - how's that progressing?


Bernd
diff mbox

Patch

Index: config/arm/thumb2.md
===================================================================
--- config/arm/thumb2.md	(revision 161893)
+++ config/arm/thumb2.md	(working copy)
@@ -200,29 +200,6 @@  (define_insn "*thumb2_neg_abssi2"
    (set_attr "length" "10,8")]
 )
 
-(define_insn "*thumb2_movdi"
-  [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, r, m")
-	(match_operand:DI 1 "di_operand"              "rDa,Db,Dc,mi,r"))]
-  "TARGET_THUMB2
-  && !(TARGET_HARD_FLOAT && (TARGET_MAVERICK || TARGET_VFP))
-  && !TARGET_IWMMXT"
-  "*
-  switch (which_alternative)
-    {
-    case 0:
-    case 1:
-    case 2:
-      return \"#\";
-    default:
-      return output_move_double (operands);
-    }
-  "
-  [(set_attr "length" "8,12,16,8,8")
-   (set_attr "type" "*,*,*,load2,store2")
-   (set_attr "pool_range" "*,*,*,4096,*")
-   (set_attr "neg_pool_range" "*,*,*,0,*")]
-)
-
 ;; We have two alternatives here for memory loads (and similarly for stores)
 ;; to reflect the fact that the permissible constant pool ranges differ
 ;; between ldr instructions taking low regs and ldr instructions taking high
@@ -269,7 +246,7 @@  (define_insn "tls_load_dot_plus_four"
 ;; Thumb-2 always has load/store halfword instructions, so we can avoid a lot
 ;; of the messiness associated with the ARM patterns.
 (define_insn "*thumb2_movhi_insn"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")    
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
 	(match_operand:HI 1 "general_operand"      "rI,n,r,m"))]
   "TARGET_THUMB2"
   "@
@@ -283,46 +260,6 @@  (define_insn "*thumb2_movhi_insn"
    (set_attr "neg_pool_range" "*,*,*,250")]
 )
 
-(define_insn "*thumb2_movsf_soft_insn"
-  [(set (match_operand:SF 0 "nonimmediate_operand" "=r,r,m")
-	(match_operand:SF 1 "general_operand"  "r,mE,r"))]
-  "TARGET_THUMB2
-   && TARGET_SOFT_FLOAT
-   && (GET_CODE (operands[0]) != MEM
-       || register_operand (operands[1], SFmode))"
-  "@
-   mov%?\\t%0, %1
-   ldr%?\\t%0, %1\\t%@ float
-   str%?\\t%1, %0\\t%@ float"
-  [(set_attr "predicable" "yes")
-   (set_attr "type" "*,load1,store1")
-   (set_attr "pool_range" "*,4096,*")
-   (set_attr "neg_pool_range" "*,0,*")]
-)
-
-(define_insn "*thumb2_movdf_soft_insn"
-  [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=r,r,r,r,m")
-	(match_operand:DF 1 "soft_df_operand" "rDa,Db,Dc,mF,r"))]
-  "TARGET_THUMB2 && TARGET_SOFT_FLOAT
-   && (   register_operand (operands[0], DFmode)
-       || register_operand (operands[1], DFmode))"
-  "*
-  switch (which_alternative)
-    {
-    case 0:
-    case 1:
-    case 2:
-      return \"#\";
-    default:
-      return output_move_double (operands);
-    }
-  "
-  [(set_attr "length" "8,12,16,8,8")
-   (set_attr "type" "*,*,*,load2,store2")
-   (set_attr "pool_range" "*,*,*,1020,*")
-   (set_attr "neg_pool_range" "*,*,*,0,*")]
-)
-
 (define_insn "*thumb2_cmpsi_shiftsi"
   [(set (reg:CC CC_REGNUM)
 	(compare:CC (match_operand:SI   0 "s_register_operand" "r")
Index: config/arm/arm.md
===================================================================
--- config/arm/arm.md	(revision 161893)
+++ config/arm/arm.md	(working copy)
@@ -169,8 +169,17 @@  (define_attr "length" "" (const_int 4))
 ; reference the pool.
 ; NEG_POOL_RANGE is nonzero for insns that can reference a constant pool entry
 ; before its address.
-(define_attr "pool_range" "" (const_int 0))
-(define_attr "neg_pool_range" "" (const_int 0))
+(define_attr "arm_pool_range" "" (const_int 0))
+(define_attr "thumb2_pool_range" "" (const_int 0))
+(define_attr "arm_neg_pool_range" "" (const_int 0))
+(define_attr "thumb2_neg_pool_range" "" (const_int 0))
+
+(define_attr "pool_range" ""
+  (cond [(eq_attr "is_thumb" "yes") (attr "thumb2_pool_range")]
+	(attr "arm_pool_range")))
+(define_attr "neg_pool_range" ""
+  (cond [(eq_attr "is_thumb" "yes") (attr "thumb2_neg_pool_range")]
+	(attr "arm_neg_pool_range")))
 
 ; An assembler sequence may clobber the condition codes without us knowing.
 ; If such an insn references the pool, then we have no way of knowing how,
@@ -4746,7 +4755,7 @@  (define_expand "movdi"
 (define_insn "*arm_movdi"
   [(set (match_operand:DI 0 "nonimmediate_di_operand" "=r, r, r, r, m")
 	(match_operand:DI 1 "di_operand"              "rDa,Db,Dc,mi,r"))]
-  "TARGET_ARM
+  "TARGET_32BIT
    && !(TARGET_HARD_FLOAT && (TARGET_MAVERICK || TARGET_VFP))
    && !TARGET_IWMMXT
    && (   register_operand (operands[0], DImode)
@@ -4764,8 +4773,10 @@  (define_insn "*arm_movdi"
   "
   [(set_attr "length" "8,12,16,8,8")
    (set_attr "type" "*,*,*,load2,store2")
-   (set_attr "pool_range" "*,*,*,1020,*")
-   (set_attr "neg_pool_range" "*,*,*,1008,*")]
+   (set_attr "arm_pool_range" "*,*,*,1020,*")
+   (set_attr "arm_neg_pool_range" "*,*,*,1008,*")
+   (set_attr "thumb2_pool_range" "*,*,*,4096,*")
+   (set_attr "thumb2_neg_pool_range" "*,*,*,0,*")]
 )
 
 (define_split
@@ -5633,7 +5644,7 @@  (define_expand "movhi_bigend"
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")    
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
 	(match_operand:HI 1 "general_operand"      "rI,K,r,m"))]
   "TARGET_ARM
    && arm_arch4
@@ -5957,7 +5968,7 @@  (define_split
 (define_insn "*arm_movsf_soft_insn"
   [(set (match_operand:SF 0 "nonimmediate_operand" "=r,r,m")
 	(match_operand:SF 1 "general_operand"  "r,mE,r"))]
-  "TARGET_ARM
+  "TARGET_32BIT
    && TARGET_SOFT_FLOAT
    && (GET_CODE (operands[0]) != MEM
        || register_operand (operands[1], SFmode))"
@@ -5965,11 +5976,11 @@  (define_insn "*arm_movsf_soft_insn"
    mov%?\\t%0, %1
    ldr%?\\t%0, %1\\t%@ float
    str%?\\t%1, %0\\t%@ float"
-  [(set_attr "length" "4,4,4")
-   (set_attr "predicable" "yes")
+  [(set_attr "predicable" "yes")
    (set_attr "type" "*,load1,store1")
    (set_attr "pool_range" "*,4096,*")
-   (set_attr "neg_pool_range" "*,4084,*")]
+   (set_attr "arm_neg_pool_range" "*,4084,*")
+   (set_attr "thumb2_neg_pool_range" "*,0,*")]
 )
 
 ;;; ??? This should have alternatives for constants.
@@ -6060,7 +6071,7 @@  (define_expand "reload_outdf"
 (define_insn "*movdf_soft_insn"
   [(set (match_operand:DF 0 "nonimmediate_soft_df_operand" "=r,r,r,r,m")
 	(match_operand:DF 1 "soft_df_operand" "rDa,Db,Dc,mF,r"))]
-  "TARGET_ARM && TARGET_SOFT_FLOAT
+  "TARGET_32BIT && TARGET_SOFT_FLOAT
    && (   register_operand (operands[0], DFmode)
        || register_operand (operands[1], DFmode))"
   "*
@@ -6076,8 +6087,9 @@  (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 "neg_pool_range" "1008")]
+   (set_attr "pool_range" "*,*,*,1020,*")
+   (set_attr "arm_neg_pool_range" "*,*,*,1008,*")
+   (set_attr "thumb2_neg_pool_range" "*,*,*,0,*")]
 )
 
 ;;; ??? This should have alternatives for constants.