public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
@ 2014-11-05  7:12 Yangfei (Felix)
  2014-11-05 11:01 ` Ramana Radhakrishnan
  0 siblings, 1 reply; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-05  7:12 UTC (permalink / raw)
  To: gcc-patches, nickc, paul, ramana.radhakrishnan, richard.earnshaw

[-- Attachment #1: Type: text/plain, Size: 3758 bytes --]

Hi,

    This patch fixes PR63742 by improving arm *movhi_insn_arch4 pattern to make it works under big-endian. 
    The idea is simple: Use movw for certain const source operand instead of ldrh.  And exclude the const values which cannot be handled by mov/mvn/movw. 
    I am doing regression test for this patch.  Assuming no issue pops up, OK for trunk? 


Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 216838)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,12 @@
+2014-11-05  Felix Yang  <felix.yang@huawei.com>
+	Shanyao Chen  <chenshanyao@huawei.com>
+
+	PR target/63742
+	* config/arm/predicates.md (arm_hi_operand): New predicate.
+	(arm_movw_immediate_operand): Similarly.
+	* config/arm/arm.md (*movhi_insn_arch4): Use arm_hi_operand instead of
+	general_operand and add "movw" to the output template.
+
 2014-10-29  Richard Sandiford  <richard.sandiford@arm.com>
 
 	* addresses.h, alias.c, asan.c, auto-inc-dec.c, bt-load.c, builtins.c,
Index: gcc/config/arm/predicates.md
===================================================================
--- gcc/config/arm/predicates.md	(revision 216838)
+++ gcc/config/arm/predicates.md	(working copy)
@@ -144,6 +144,12 @@
   (and (match_code "const_int")
        (match_test "INTVAL (op) == 0")))
 
+(define_predicate "arm_movw_immediate_operand"
+  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
+       (ior (match_code "high")
+            (and (match_code "const_int")
+                 (match_test "(INTVAL (op) & 0xffff0000) == 0")))))
+
 ;; Something valid on the RHS of an ARM data-processing instruction
 (define_predicate "arm_rhs_operand"
   (ior (match_operand 0 "s_register_operand")
@@ -211,6 +217,11 @@
   (ior (match_operand 0 "arm_rhs_operand")
        (match_operand 0 "arm_not_immediate_operand")))
 
+(define_predicate "arm_hi_operand"
+  (ior (match_operand 0 "arm_rhsm_operand")
+       (ior (match_operand 0 "arm_not_immediate_operand")
+            (match_operand 0 "arm_movw_immediate_operand"))))
+
 (define_predicate "arm_di_operand"
   (ior (match_operand 0 "s_register_operand")
        (match_operand 0 "arm_immediate_di_operand")))
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 216838)
+++ gcc/config/arm/arm.md	(working copy)
@@ -6285,8 +6285,8 @@
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
-	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
+	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
   "TARGET_ARM
    && arm_arch4
    && (register_operand (operands[0], HImode)
@@ -6294,16 +6294,18 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
+   movw%?\\t%0, %1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,256")
-   (set_attr "neg_pool_range" "*,*,*,244")
+   (set_attr "pool_range" "*,*,*,*,256")
+   (set_attr "neg_pool_range" "*,*,*,*,244")
    (set_attr_alternative "type"
                          [(if_then_else (match_operand 1 "const_int_operand" "")
                                         (const_string "mov_imm" )
                                         (const_string "mov_reg"))
                           (const_string "mvn_imm")
+                          (const_string "mov_imm")
                           (const_string "store1")
                           (const_string "load1")])]
 )

[-- Attachment #2: movhi_insn_arch4-fix-v1.patch --]
[-- Type: application/octet-stream, Size: 3309 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 216838)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,12 @@
+2014-11-05  Felix Yang  <felix.yang@huawei.com>
+	Shanyao Chen  <chenshanyao@huawei.com>
+
+	PR target/63742
+	* config/arm/predicates.md (arm_hi_operand): New predicate.
+	(arm_movw_immediate_operand): Similarly.
+	* config/arm/arm.md (*movhi_insn_arch4): Use arm_hi_operand instead of
+	general_operand and add "movw" to the output template.
+
 2014-10-29  Richard Sandiford  <richard.sandiford@arm.com>
 
 	* addresses.h, alias.c, asan.c, auto-inc-dec.c, bt-load.c, builtins.c,
Index: gcc/config/arm/predicates.md
===================================================================
--- gcc/config/arm/predicates.md	(revision 216838)
+++ gcc/config/arm/predicates.md	(working copy)
@@ -144,6 +144,12 @@
   (and (match_code "const_int")
        (match_test "INTVAL (op) == 0")))
 
+(define_predicate "arm_movw_immediate_operand"
+  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
+       (ior (match_code "high")
+            (and (match_code "const_int")
+                 (match_test "(INTVAL (op) & 0xffff0000) == 0")))))
+
 ;; Something valid on the RHS of an ARM data-processing instruction
 (define_predicate "arm_rhs_operand"
   (ior (match_operand 0 "s_register_operand")
@@ -211,6 +217,11 @@
   (ior (match_operand 0 "arm_rhs_operand")
        (match_operand 0 "arm_not_immediate_operand")))
 
+(define_predicate "arm_hi_operand"
+  (ior (match_operand 0 "arm_rhsm_operand")
+       (ior (match_operand 0 "arm_not_immediate_operand")
+            (match_operand 0 "arm_movw_immediate_operand"))))
+
 (define_predicate "arm_di_operand"
   (ior (match_operand 0 "s_register_operand")
        (match_operand 0 "arm_immediate_di_operand")))
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 216838)
+++ gcc/config/arm/arm.md	(working copy)
@@ -6285,8 +6285,8 @@
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
-	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
+	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
   "TARGET_ARM
    && arm_arch4
    && (register_operand (operands[0], HImode)
@@ -6294,16 +6294,18 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
+   movw%?\\t%0, %1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,256")
-   (set_attr "neg_pool_range" "*,*,*,244")
+   (set_attr "pool_range" "*,*,*,*,256")
+   (set_attr "neg_pool_range" "*,*,*,*,244")
    (set_attr_alternative "type"
                          [(if_then_else (match_operand 1 "const_int_operand" "")
                                         (const_string "mov_imm" )
                                         (const_string "mov_reg"))
                           (const_string "mvn_imm")
+                          (const_string "mov_imm")
                           (const_string "store1")
                           (const_string "load1")])]
 )

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-05  7:12 [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian Yangfei (Felix)
@ 2014-11-05 11:01 ` Ramana Radhakrishnan
  2014-11-06  8:36   ` Yangfei (Felix)
  2014-11-12 10:28   ` [PING][PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian Yangfei (Felix)
  0 siblings, 2 replies; 17+ messages in thread
From: Ramana Radhakrishnan @ 2014-11-05 11:01 UTC (permalink / raw)
  To: Yangfei (Felix), gcc-patches, nickc, paul, Richard Earnshaw



On 05/11/14 07:09, Yangfei (Felix) wrote:
> Hi,
>
>      This patch fixes PR63742 by improving arm *movhi_insn_arch4 pattern to make it works under big-endian.
>      The idea is simple: Use movw for certain const source operand instead of ldrh.  And exclude the const values which cannot be handled by mov/mvn/movw.
>      I am doing regression test for this patch.  Assuming no issue pops up, OK for trunk?

So, doesn't that makes the bug latent for architectures older than 
armv6t2 and big endian and only fixed this in ARM state ? I'd prefer a 
complete solution please. What about *thumb2_movhi_insn in thumb2.md ?

>
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog	(revision 216838)
> +++ gcc/ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> +2014-11-05  Felix Yang  <felix.yang@huawei.com>
> +	Shanyao Chen  <chenshanyao@huawei.com>
> +

I'm assuming you have copyright assignments sorted.


> +	PR target/63742
> +	* config/arm/predicates.md (arm_hi_operand): New predicate.
> +	(arm_movw_immediate_operand): Similarly.
> +	* config/arm/arm.md (*movhi_insn_arch4): Use arm_hi_operand instead of
> +	general_operand and add "movw" to the output template.
> +
>   2014-10-29  Richard Sandiford  <richard.sandiford@arm.com>
>
>   	* addresses.h, alias.c, asan.c, auto-inc-dec.c, bt-load.c, builtins.c,
> Index: gcc/config/arm/predicates.md
> ===================================================================
> --- gcc/config/arm/predicates.md	(revision 216838)
> +++ gcc/config/arm/predicates.md	(working copy)
> @@ -144,6 +144,12 @@
>     (and (match_code "const_int")
>          (match_test "INTVAL (op) == 0")))
>
> +(define_predicate "arm_movw_immediate_operand"
> +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> +       (ior (match_code "high")

I don't see why you need to check for "high" here ?

> +            (and (match_code "const_int")
> +                 (match_test "(INTVAL (op) & 0xffff0000) == 0")))))
> +
>   ;; Something valid on the RHS of an ARM data-processing instruction
>   (define_predicate "arm_rhs_operand"
>     (ior (match_operand 0 "s_register_operand")
> @@ -211,6 +217,11 @@
>     (ior (match_operand 0 "arm_rhs_operand")
>          (match_operand 0 "arm_not_immediate_operand")))
>
> +(define_predicate "arm_hi_operand"
> +  (ior (match_operand 0 "arm_rhsm_operand")
> +       (ior (match_operand 0 "arm_not_immediate_operand")
> +            (match_operand 0 "arm_movw_immediate_operand"))))
> +
>   (define_predicate "arm_di_operand"
>     (ior (match_operand 0 "s_register_operand")
>          (match_operand 0 "arm_immediate_di_operand")))
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md	(revision 216838)
> +++ gcc/config/arm/arm.md	(working copy)
> @@ -6285,8 +6285,8 @@
>
>   ;; Pattern to recognize insn generated default case above
>   (define_insn "*movhi_insn_arch4"
> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
> -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
> +	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
>     "TARGET_ARM
>      && arm_arch4
>      && (register_operand (operands[0], HImode)
> @@ -6294,16 +6294,18 @@
>     "@
>      mov%?\\t%0, %1\\t%@ movhi
>      mvn%?\\t%0, #%B1\\t%@ movhi
> +   movw%?\\t%0, %1\\t%@ movhi
>      str%(h%)\\t%1, %0\\t%@ movhi
>      ldr%(h%)\\t%0, %1\\t%@ movhi"
>     [(set_attr "predicable" "yes")
> -   (set_attr "pool_range" "*,*,*,256")
> -   (set_attr "neg_pool_range" "*,*,*,244")
> +   (set_attr "pool_range" "*,*,*,*,256")
> +   (set_attr "neg_pool_range" "*,*,*,*,244")
>      (set_attr_alternative "type"
>                            [(if_then_else (match_operand 1 "const_int_operand" "")
>                                           (const_string "mov_imm" )
>                                           (const_string "mov_reg"))
>                             (const_string "mvn_imm")
> +                          (const_string "mov_imm")
>                             (const_string "store1")
>                             (const_string "load1")])]
>   )
>

Ramana

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-05 11:01 ` Ramana Radhakrishnan
@ 2014-11-06  8:36   ` Yangfei (Felix)
  2014-11-18  8:54     ` Ramana Radhakrishnan
  2014-11-12 10:28   ` [PING][PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian Yangfei (Felix)
  1 sibling, 1 reply; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-06  8:36 UTC (permalink / raw)
  To: Ramana Radhakrishnan, gcc-patches, nickc, paul, Richard Earnshaw
  Cc: Chenshanyao

[-- Attachment #1: Type: text/plain, Size: 5045 bytes --]

> >      The idea is simple: Use movw for certain const source operand instead of
> ldrh.  And exclude the const values which cannot be handled by
> mov/mvn/movw.
> >      I am doing regression test for this patch.  Assuming no issue pops up,
> OK for trunk?
> 
> So, doesn't that makes the bug latent for architectures older than
> armv6t2 and big endian and only fixed this in ARM state ? I'd prefer a complete
> solution please. What about *thumb2_movhi_insn in thumb2.md ?
> 

Actually, we fix the bug by excluding the const values which cannot be handled. The patch still works even without the adding of "movw" here. 
The new "movw" alternative here is just an small code optimization for certain arch. We can handle consts by movw instead of ldrh and this better for performance. 
We find that this bug is not reproducible for *thumb2_movhi_insn. The reason is that this pattern can always move consts using "movw". 


> > --- gcc/ChangeLog	(revision 216838)
> > +++ gcc/ChangeLog	(working copy)
> > @@ -1,3 +1,12 @@
> > +2014-11-05  Felix Yang  <felix.yang@huawei.com>
> > +	Shanyao Chen  <chenshanyao@huawei.com>
> > +
> 
> I'm assuming you have copyright assignments sorted.

Yes, both my employer(Huawei) and I have signed copyright assignments with FSF. 


> > +(define_predicate "arm_movw_immediate_operand"
> > +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> > +       (ior (match_code "high")
> 
> I don't see why you need to check for "high" here ?
> 

Yeah, I double checked and found that it's not necessary here. Thanks for pointing this out. 
Attached please find the updated patch. Reg-tested for armeb-none-eabi. 
OK for the trunk? 


Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 216838)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,12 @@
+2014-11-05  Felix Yang  <felix.yang@huawei.com>
+	Shanyao Chen  <chenshanyao@huawei.com>
+
+	PR target/63742
+	* config/arm/predicates.md (arm_hi_operand): New predicate.
+	(arm_movw_immediate_operand): Similarly.
+	* config/arm/arm.md (*movhi_insn_arch4): Use arm_hi_operand instead of
+	general_operand and add "movw" to the output template.
+
 2014-10-29  Richard Sandiford  <richard.sandiford@arm.com>
 
 	* addresses.h, alias.c, asan.c, auto-inc-dec.c, bt-load.c, builtins.c,
Index: gcc/config/arm/predicates.md
===================================================================
--- gcc/config/arm/predicates.md	(revision 216838)
+++ gcc/config/arm/predicates.md	(working copy)
@@ -144,6 +144,11 @@
   (and (match_code "const_int")
        (match_test "INTVAL (op) == 0")))
 
+(define_predicate "arm_movw_immediate_operand"
+  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
+       (and (match_code "const_int")
+	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
+
 ;; Something valid on the RHS of an ARM data-processing instruction
 (define_predicate "arm_rhs_operand"
   (ior (match_operand 0 "s_register_operand")
@@ -211,6 +216,11 @@
   (ior (match_operand 0 "arm_rhs_operand")
        (match_operand 0 "arm_not_immediate_operand")))
 
+(define_predicate "arm_hi_operand"
+  (ior (match_operand 0 "arm_rhsm_operand")
+       (ior (match_operand 0 "arm_not_immediate_operand")
+            (match_operand 0 "arm_movw_immediate_operand"))))
+
 (define_predicate "arm_di_operand"
   (ior (match_operand 0 "s_register_operand")
        (match_operand 0 "arm_immediate_di_operand")))
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 216838)
+++ gcc/config/arm/arm.md	(working copy)
@@ -6285,8 +6285,8 @@
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
-	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
+	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
   "TARGET_ARM
    && arm_arch4
    && (register_operand (operands[0], HImode)
@@ -6294,16 +6294,18 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
+   movw%?\\t%0, %1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,256")
-   (set_attr "neg_pool_range" "*,*,*,244")
+   (set_attr "pool_range" "*,*,*,*,256")
+   (set_attr "neg_pool_range" "*,*,*,*,244")
    (set_attr_alternative "type"
                          [(if_then_else (match_operand 1 "const_int_operand" "")
                                         (const_string "mov_imm" )
                                         (const_string "mov_reg"))
                           (const_string "mvn_imm")
+                          (const_string "mov_imm")
                           (const_string "store1")
                           (const_string "load1")])]
 )

[-- Attachment #2: movhi_insn_arch4-fix-v2.patch --]
[-- Type: application/octet-stream, Size: 3258 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 216838)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,12 @@
+2014-11-05  Felix Yang  <felix.yang@huawei.com>
+	Shanyao Chen  <chenshanyao@huawei.com>
+
+	PR target/63742
+	* config/arm/predicates.md (arm_hi_operand): New predicate.
+	(arm_movw_immediate_operand): Similarly.
+	* config/arm/arm.md (*movhi_insn_arch4): Use arm_hi_operand instead of
+	general_operand and add "movw" to the output template.
+
 2014-10-29  Richard Sandiford  <richard.sandiford@arm.com>
 
 	* addresses.h, alias.c, asan.c, auto-inc-dec.c, bt-load.c, builtins.c,
Index: gcc/config/arm/predicates.md
===================================================================
--- gcc/config/arm/predicates.md	(revision 216838)
+++ gcc/config/arm/predicates.md	(working copy)
@@ -144,6 +144,11 @@
   (and (match_code "const_int")
        (match_test "INTVAL (op) == 0")))
 
+(define_predicate "arm_movw_immediate_operand"
+  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
+       (and (match_code "const_int")
+	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
+
 ;; Something valid on the RHS of an ARM data-processing instruction
 (define_predicate "arm_rhs_operand"
   (ior (match_operand 0 "s_register_operand")
@@ -211,6 +216,11 @@
   (ior (match_operand 0 "arm_rhs_operand")
        (match_operand 0 "arm_not_immediate_operand")))
 
+(define_predicate "arm_hi_operand"
+  (ior (match_operand 0 "arm_rhsm_operand")
+       (ior (match_operand 0 "arm_not_immediate_operand")
+            (match_operand 0 "arm_movw_immediate_operand"))))
+
 (define_predicate "arm_di_operand"
   (ior (match_operand 0 "s_register_operand")
        (match_operand 0 "arm_immediate_di_operand")))
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 216838)
+++ gcc/config/arm/arm.md	(working copy)
@@ -6285,8 +6285,8 @@
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
-	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
+	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
   "TARGET_ARM
    && arm_arch4
    && (register_operand (operands[0], HImode)
@@ -6294,16 +6294,18 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
+   movw%?\\t%0, %1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,256")
-   (set_attr "neg_pool_range" "*,*,*,244")
+   (set_attr "pool_range" "*,*,*,*,256")
+   (set_attr "neg_pool_range" "*,*,*,*,244")
    (set_attr_alternative "type"
                          [(if_then_else (match_operand 1 "const_int_operand" "")
                                         (const_string "mov_imm" )
                                         (const_string "mov_reg"))
                           (const_string "mvn_imm")
+                          (const_string "mov_imm")
                           (const_string "store1")
                           (const_string "load1")])]
 )

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PING][PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-05 11:01 ` Ramana Radhakrishnan
  2014-11-06  8:36   ` Yangfei (Felix)
@ 2014-11-12 10:28   ` Yangfei (Felix)
  1 sibling, 0 replies; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-12 10:28 UTC (permalink / raw)
  To: Ramana Radhakrishnan, gcc-patches, nickc, paul, Richard Earnshaw
  Cc: Chenshanyao

Hello,

    Any comments on this patch?  Thanks. 



> > >      The idea is simple: Use movw for certain const source operand
> > > instead of
> > ldrh.  And exclude the const values which cannot be handled by
> > mov/mvn/movw.
> > >      I am doing regression test for this patch.  Assuming no issue
> > > pops up,
> > OK for trunk?
> >
> > So, doesn't that makes the bug latent for architectures older than
> > armv6t2 and big endian and only fixed this in ARM state ? I'd prefer a
> > complete solution please. What about *thumb2_movhi_insn in thumb2.md ?
> >
> 
> Actually, we fix the bug by excluding the const values which cannot be handled.
> The patch still works even without the adding of "movw" here.
> The new "movw" alternative here is just an small code optimization for certain
> arch. We can handle consts by movw instead of ldrh and this better for
> performance.
> We find that this bug is not reproducible for *thumb2_movhi_insn. The reason is
> that this pattern can always move consts using "movw".
> 
> 
> > > --- gcc/ChangeLog	(revision 216838)
> > > +++ gcc/ChangeLog	(working copy)
> > > @@ -1,3 +1,12 @@
> > > +2014-11-05  Felix Yang  <felix.yang@huawei.com>
> > > +	Shanyao Chen  <chenshanyao@huawei.com>
> > > +
> >
> > I'm assuming you have copyright assignments sorted.
> 
> Yes, both my employer(Huawei) and I have signed copyright assignments with
> FSF.
> 
> 
> > > +(define_predicate "arm_movw_immediate_operand"
> > > +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> > > +       (ior (match_code "high")
> >
> > I don't see why you need to check for "high" here ?
> >
> 
> Yeah, I double checked and found that it's not necessary here. Thanks for
> pointing this out.
> Attached please find the updated patch. Reg-tested for armeb-none-eabi.
> OK for the trunk?
> 
> 
> Index: gcc/ChangeLog
> =============================================================
> ======
> --- gcc/ChangeLog	(revision 216838)
> +++ gcc/ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> +2014-11-05  Felix Yang  <felix.yang@huawei.com>
> +	Shanyao Chen  <chenshanyao@huawei.com>
> +
> +	PR target/63742
> +	* config/arm/predicates.md (arm_hi_operand): New predicate.
> +	(arm_movw_immediate_operand): Similarly.
> +	* config/arm/arm.md (*movhi_insn_arch4): Use arm_hi_operand instead
> of
> +	general_operand and add "movw" to the output template.
> +
>  2014-10-29  Richard Sandiford  <richard.sandiford@arm.com>
> 
>  	* addresses.h, alias.c, asan.c, auto-inc-dec.c, bt-load.c, builtins.c,
> Index: gcc/config/arm/predicates.md
> =============================================================
> ======
> --- gcc/config/arm/predicates.md	(revision 216838)
> +++ gcc/config/arm/predicates.md	(working copy)
> @@ -144,6 +144,11 @@
>    (and (match_code "const_int")
>         (match_test "INTVAL (op) == 0")))
> 
> +(define_predicate "arm_movw_immediate_operand"
> +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> +       (and (match_code "const_int")
> +	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
> +
>  ;; Something valid on the RHS of an ARM data-processing instruction
> (define_predicate "arm_rhs_operand"
>    (ior (match_operand 0 "s_register_operand") @@ -211,6 +216,11 @@
>    (ior (match_operand 0 "arm_rhs_operand")
>         (match_operand 0 "arm_not_immediate_operand")))
> 
> +(define_predicate "arm_hi_operand"
> +  (ior (match_operand 0 "arm_rhsm_operand")
> +       (ior (match_operand 0 "arm_not_immediate_operand")
> +            (match_operand 0 "arm_movw_immediate_operand"))))
> +
>  (define_predicate "arm_di_operand"
>    (ior (match_operand 0 "s_register_operand")
>         (match_operand 0 "arm_immediate_di_operand")))
> Index: gcc/config/arm/arm.md
> =============================================================
> ======
> --- gcc/config/arm/arm.md	(revision 216838)
> +++ gcc/config/arm/arm.md	(working copy)
> @@ -6285,8 +6285,8 @@
> 
>  ;; Pattern to recognize insn generated default case above  (define_insn
> "*movhi_insn_arch4"
> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
> -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
> +	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
>    "TARGET_ARM
>     && arm_arch4
>     && (register_operand (operands[0], HImode) @@ -6294,16 +6294,18 @@
>    "@
>     mov%?\\t%0, %1\\t%@ movhi
>     mvn%?\\t%0, #%B1\\t%@ movhi
> +   movw%?\\t%0, %1\\t%@ movhi
>     str%(h%)\\t%1, %0\\t%@ movhi
>     ldr%(h%)\\t%0, %1\\t%@ movhi"
>    [(set_attr "predicable" "yes")
> -   (set_attr "pool_range" "*,*,*,256")
> -   (set_attr "neg_pool_range" "*,*,*,244")
> +   (set_attr "pool_range" "*,*,*,*,256")
> +   (set_attr "neg_pool_range" "*,*,*,*,244")
>     (set_attr_alternative "type"
>                           [(if_then_else (match_operand 1
> "const_int_operand" "")
>                                          (const_string "mov_imm" )
>                                          (const_string "mov_reg"))
>                            (const_string "mvn_imm")
> +                          (const_string "mov_imm")
>                            (const_string "store1")
>                            (const_string "load1")])]
>  )

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-06  8:36   ` Yangfei (Felix)
@ 2014-11-18  8:54     ` Ramana Radhakrishnan
  2014-11-18 11:36       ` Yangfei (Felix)
  0 siblings, 1 reply; 17+ messages in thread
From: Ramana Radhakrishnan @ 2014-11-18  8:54 UTC (permalink / raw)
  To: Yangfei (Felix), gcc-patches, nickc, paul, Richard Earnshaw; +Cc: Chenshanyao



On 06/11/14 08:35, Yangfei (Felix) wrote:
>>>       The idea is simple: Use movw for certain const source operand instead of
>> ldrh.  And exclude the const values which cannot be handled by
>> mov/mvn/movw.
>>>       I am doing regression test for this patch.  Assuming no issue pops up,
>> OK for trunk?
>>
>> So, doesn't that makes the bug latent for architectures older than
>> armv6t2 and big endian and only fixed this in ARM state ? I'd prefer a complete
>> solution please. What about *thumb2_movhi_insn in thumb2.md ?
>>
>
> Actually, we fix the bug by excluding the const values which cannot be handled. The patch still works even without the adding of "movw" here.
> The new "movw" alternative here is just an small code optimization for certain arch. We can handle consts by movw instead of ldrh and this better for performance.
> We find that this bug is not reproducible for *thumb2_movhi_insn. The reason is that this pattern can always move consts using "movw".

Please fix the PR number in your final commit with PR 59593.

> Index: gcc/config/arm/predicates.md
> ===================================================================
> --- gcc/config/arm/predicates.md	(revision 216838)
> +++ gcc/config/arm/predicates.md	(working copy)
> @@ -144,6 +144,11 @@
>    (and (match_code "const_int")
>         (match_test "INTVAL (op) == 0")))
>
> +(define_predicate "arm_movw_immediate_operand"
> +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> +       (and (match_code "const_int")
> +	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
> +
>  ;; Something valid on the RHS of an ARM data-processing instruction
>  (define_predicate "arm_rhs_operand"
>    (ior (match_operand 0 "s_register_operand")
> @@ -211,6 +216,11 @@
>    (ior (match_operand 0 "arm_rhs_operand")
>         (match_operand 0 "arm_not_immediate_operand")))
>
> +(define_predicate "arm_hi_operand"
> +  (ior (match_operand 0 "arm_rhsm_operand")
> +       (ior (match_operand 0 "arm_not_immediate_operand")
> +            (match_operand 0 "arm_movw_immediate_operand"))))
> +

I think you don't need any of these predicates.


>  (define_predicate "arm_di_operand"
>    (ior (match_operand 0 "s_register_operand")
>         (match_operand 0 "arm_immediate_di_operand")))
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md	(revision 216838)
> +++ gcc/config/arm/arm.md	(working copy)
> @@ -6285,8 +6285,8 @@
>
>  ;; Pattern to recognize insn generated default case above
>  (define_insn "*movhi_insn_arch4"
> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
> -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
> +	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]

Use `n' instead of `j' - movw can handle all of the numerical constants 
for HImode values. And the predicate can remain general_operand.

>    "TARGET_ARM
>     && arm_arch4
>     && (register_operand (operands[0], HImode)
> @@ -6294,16 +6294,18 @@
>    "@
>     mov%?\\t%0, %1\\t%@ movhi
>     mvn%?\\t%0, #%B1\\t%@ movhi
> +   movw%?\\t%0, %1\\t%@ movhi
>     str%(h%)\\t%1, %0\\t%@ movhi
>     ldr%(h%)\\t%0, %1\\t%@ movhi"
>    [(set_attr "predicable" "yes")
> -   (set_attr "pool_range" "*,*,*,256")
> -   (set_attr "neg_pool_range" "*,*,*,244")
> +   (set_attr "pool_range" "*,*,*,*,256")
> +   (set_attr "neg_pool_range" "*,*,*,*,244")
>     (set_attr_alternative "type"
>                           [(if_then_else (match_operand 1 "const_int_operand" "")
>                                          (const_string "mov_imm" )
>                                          (const_string "mov_reg"))
>                            (const_string "mvn_imm")
> +                          (const_string "mov_imm")
>                            (const_string "store1")
>                            (const_string "load1")])]
>  )

Look at the set_attr "arch" alternative and set this to t2 for the movw 
alternative. I would expect that to be enough to sort this out instead 
of adding all this code.

I don't think your patch or the one with my modifications fixes the 
issue completely for architectures that do not have movw / movt 
instructions and I would expect that one would still need Thomas's patch 
to create const table entries of the right size.

Otherwise this is OK.


regards
Ramana

>
>
>>> --- gcc/ChangeLog	(revision 216838)
>>> +++ gcc/ChangeLog	(working copy)
>>> @@ -1,3 +1,12 @@
>>> +2014-11-05  Felix Yang  <felix.yang@huawei.com>
>>> +	Shanyao Chen  <chenshanyao@huawei.com>
>>> +
>>
>> I'm assuming you have copyright assignments sorted.
>
> Yes, both my employer(Huawei) and I have signed copyright assignments with FSF.
>
>
>>> +(define_predicate "arm_movw_immediate_operand"
>>> +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
>>> +       (ior (match_code "high")
>>
>> I don't see why you need to check for "high" here ?
>>
>
> Yeah, I double checked and found that it's not necessary here. Thanks for pointing this out.
> Attached please find the updated patch. Reg-tested for armeb-none-eabi.
> OK for the trunk?
>
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog	(revision 216838)
> +++ gcc/ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> +2014-11-05  Felix Yang  <felix.yang@huawei.com>
> +	Shanyao Chen  <chenshanyao@huawei.com>
> +
> +	PR target/63742
> +	* config/arm/predicates.md (arm_hi_operand): New predicate.
> +	(arm_movw_immediate_operand): Similarly.
> +	* config/arm/arm.md (*movhi_insn_arch4): Use arm_hi_operand instead of
> +	general_operand and add "movw" to the output template.
> +
>   2014-10-29  Richard Sandiford  <richard.sandiford@arm.com>
>
>   	* addresses.h, alias.c, asan.c, auto-inc-dec.c, bt-load.c, builtins.c,
> Index: gcc/config/arm/predicates.md
> ===================================================================
> --- gcc/config/arm/predicates.md	(revision 216838)
> +++ gcc/config/arm/predicates.md	(working copy)
> @@ -144,6 +144,11 @@
>     (and (match_code "const_int")
>          (match_test "INTVAL (op) == 0")))
>
> +(define_predicate "arm_movw_immediate_operand"
> +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> +       (and (match_code "const_int")
> +	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
> +
>   ;; Something valid on the RHS of an ARM data-processing instruction
>   (define_predicate "arm_rhs_operand"
>     (ior (match_operand 0 "s_register_operand")
> @@ -211,6 +216,11 @@
>     (ior (match_operand 0 "arm_rhs_operand")
>          (match_operand 0 "arm_not_immediate_operand")))
>
> +(define_predicate "arm_hi_operand"
> +  (ior (match_operand 0 "arm_rhsm_operand")
> +       (ior (match_operand 0 "arm_not_immediate_operand")
> +            (match_operand 0 "arm_movw_immediate_operand"))))
> +
>   (define_predicate "arm_di_operand"
>     (ior (match_operand 0 "s_register_operand")
>          (match_operand 0 "arm_immediate_di_operand")))
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md	(revision 216838)
> +++ gcc/config/arm/arm.md	(working copy)
> @@ -6285,8 +6285,8 @@
>
>   ;; Pattern to recognize insn generated default case above
>   (define_insn "*movhi_insn_arch4"
> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
> -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
> +	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
>     "TARGET_ARM
>      && arm_arch4
>      && (register_operand (operands[0], HImode)
> @@ -6294,16 +6294,18 @@
>     "@
>      mov%?\\t%0, %1\\t%@ movhi
>      mvn%?\\t%0, #%B1\\t%@ movhi
> +   movw%?\\t%0, %1\\t%@ movhi
>      str%(h%)\\t%1, %0\\t%@ movhi
>      ldr%(h%)\\t%0, %1\\t%@ movhi"
>     [(set_attr "predicable" "yes")
> -   (set_attr "pool_range" "*,*,*,256")
> -   (set_attr "neg_pool_range" "*,*,*,244")
> +   (set_attr "pool_range" "*,*,*,*,256")
> +   (set_attr "neg_pool_range" "*,*,*,*,244")
>      (set_attr_alternative "type"
>                            [(if_then_else (match_operand 1 "const_int_operand" "")
>                                           (const_string "mov_imm" )
>                                           (const_string "mov_reg"))
>                             (const_string "mvn_imm")
> +                          (const_string "mov_imm")
>                             (const_string "store1")
>                             (const_string "load1")])]
>   )
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-18  8:54     ` Ramana Radhakrishnan
@ 2014-11-18 11:36       ` Yangfei (Felix)
  2014-11-18 12:04         ` Ramana Radhakrishnan
  0 siblings, 1 reply; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-18 11:36 UTC (permalink / raw)
  To: Ramana Radhakrishnan, gcc-patches, nickc, paul, Richard Earnshaw
  Cc: Chenshanyao

> On 06/11/14 08:35, Yangfei (Felix) wrote:
> >>>       The idea is simple: Use movw for certain const source operand
> >>> instead of
> >> ldrh.  And exclude the const values which cannot be handled by
> >> mov/mvn/movw.
> >>>       I am doing regression test for this patch.  Assuming no issue
> >>> pops up,
> >> OK for trunk?
> >>
> >> So, doesn't that makes the bug latent for architectures older than
> >> armv6t2 and big endian and only fixed this in ARM state ? I'd prefer
> >> a complete solution please. What about *thumb2_movhi_insn in
> thumb2.md ?
> >>
> >
> > Actually, we fix the bug by excluding the const values which cannot be handled.
> The patch still works even without the adding of "movw" here.
> > The new "movw" alternative here is just an small code optimization for certain
> arch. We can handle consts by movw instead of ldrh and this better for
> performance.
> > We find that this bug is not reproducible for *thumb2_movhi_insn. The reason
> is that this pattern can always move consts using "movw".
> 
> Please fix the PR number in your final commit with PR 59593.
> 
> > Index: gcc/config/arm/predicates.md
> >
> =============================================================
> ======
> > --- gcc/config/arm/predicates.md	(revision 216838)
> > +++ gcc/config/arm/predicates.md	(working copy)
> > @@ -144,6 +144,11 @@
> >    (and (match_code "const_int")
> >         (match_test "INTVAL (op) == 0")))
> >
> > +(define_predicate "arm_movw_immediate_operand"
> > +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> > +       (and (match_code "const_int")
> > +	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
> > +
> >  ;; Something valid on the RHS of an ARM data-processing instruction
> > (define_predicate "arm_rhs_operand"
> >    (ior (match_operand 0 "s_register_operand") @@ -211,6 +216,11 @@
> >    (ior (match_operand 0 "arm_rhs_operand")
> >         (match_operand 0 "arm_not_immediate_operand")))
> >
> > +(define_predicate "arm_hi_operand"
> > +  (ior (match_operand 0 "arm_rhsm_operand")
> > +       (ior (match_operand 0 "arm_not_immediate_operand")
> > +            (match_operand 0 "arm_movw_immediate_operand"))))
> > +
> 
> I think you don't need any of these predicates.
> 
> 
> >  (define_predicate "arm_di_operand"
> >    (ior (match_operand 0 "s_register_operand")
> >         (match_operand 0 "arm_immediate_di_operand")))
> > Index: gcc/config/arm/arm.md
> >
> =============================================================
> ======
> > --- gcc/config/arm/arm.md	(revision 216838)
> > +++ gcc/config/arm/arm.md	(working copy)
> > @@ -6285,8 +6285,8 @@
> >
> >  ;; Pattern to recognize insn generated default case above
> > (define_insn "*movhi_insn_arch4"
> > -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
> > -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
> > +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
> > +	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
> 
> Use `n' instead of `j' - movw can handle all of the numerical constants for HImode
> values. And the predicate can remain general_operand.
> 


Hello Ramana,

  We need to make sure that movw is only used for architectures which satisfy arm_arch_thumb2 as indicated in the following predicate added:

+(define_predicate "arm_movw_immediate_operand"
+  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
+       (and (match_code "const_int")
+	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))

  I am modifying the predicate in order to fix issue for other older architectures.
  It seems we can't achieve this by simply using 'n' instead of 'j' here, right?
  Thanks.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-18 11:36       ` Yangfei (Felix)
@ 2014-11-18 12:04         ` Ramana Radhakrishnan
  2014-11-18 12:47           ` Yangfei (Felix)
  0 siblings, 1 reply; 17+ messages in thread
From: Ramana Radhakrishnan @ 2014-11-18 12:04 UTC (permalink / raw)
  To: Yangfei (Felix), gcc-patches, nickc, paul, Richard Earnshaw; +Cc: Chenshanyao



On 18/11/14 11:02, Yangfei (Felix) wrote:
>> On 06/11/14 08:35, Yangfei (Felix) wrote:
>>>>>        The idea is simple: Use movw for certain const source operand
>>>>> instead of
>>>> ldrh.  And exclude the const values which cannot be handled by
>>>> mov/mvn/movw.
>>>>>        I am doing regression test for this patch.  Assuming no issue
>>>>> pops up,
>>>> OK for trunk?
>>>>
>>>> So, doesn't that makes the bug latent for architectures older than
>>>> armv6t2 and big endian and only fixed this in ARM state ? I'd prefer
>>>> a complete solution please. What about *thumb2_movhi_insn in
>> thumb2.md ?
>>>>
>>>
>>> Actually, we fix the bug by excluding the const values which cannot be handled.
>> The patch still works even without the adding of "movw" here.
>>> The new "movw" alternative here is just an small code optimization for certain
>> arch. We can handle consts by movw instead of ldrh and this better for
>> performance.
>>> We find that this bug is not reproducible for *thumb2_movhi_insn. The reason
>> is that this pattern can always move consts using "movw".
>>
>> Please fix the PR number in your final commit with PR 59593.
>>
>>> Index: gcc/config/arm/predicates.md
>>>
>> =============================================================
>> ======
>>> --- gcc/config/arm/predicates.md	(revision 216838)
>>> +++ gcc/config/arm/predicates.md	(working copy)
>>> @@ -144,6 +144,11 @@
>>>     (and (match_code "const_int")
>>>          (match_test "INTVAL (op) == 0")))
>>>
>>> +(define_predicate "arm_movw_immediate_operand"
>>> +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
>>> +       (and (match_code "const_int")
>>> +	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
>>> +
>>>   ;; Something valid on the RHS of an ARM data-processing instruction
>>> (define_predicate "arm_rhs_operand"
>>>     (ior (match_operand 0 "s_register_operand") @@ -211,6 +216,11 @@
>>>     (ior (match_operand 0 "arm_rhs_operand")
>>>          (match_operand 0 "arm_not_immediate_operand")))
>>>
>>> +(define_predicate "arm_hi_operand"
>>> +  (ior (match_operand 0 "arm_rhsm_operand")
>>> +       (ior (match_operand 0 "arm_not_immediate_operand")
>>> +            (match_operand 0 "arm_movw_immediate_operand"))))
>>> +
>>
>> I think you don't need any of these predicates.
>>
>>
>>>   (define_predicate "arm_di_operand"
>>>     (ior (match_operand 0 "s_register_operand")
>>>          (match_operand 0 "arm_immediate_di_operand")))
>>> Index: gcc/config/arm/arm.md
>>>
>> =============================================================
>> ======
>>> --- gcc/config/arm/arm.md	(revision 216838)
>>> +++ gcc/config/arm/arm.md	(working copy)
>>> @@ -6285,8 +6285,8 @@
>>>
>>>   ;; Pattern to recognize insn generated default case above
>>> (define_insn "*movhi_insn_arch4"
>>> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
>>> -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
>>> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
>>> +	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
>>
>> Use `n' instead of `j' - movw can handle all of the numerical constants for HImode
>> values. And the predicate can remain general_operand.
>>

Did you read my comment about set_attr "arch" further down in the thread ?


> Look at the set_attr "arch" alternative and set this to t2 for the movw alternative. I would expect that to be enough to sort this out instead of adding all this code.




Ramana

>
>
> Hello Ramana,
>
>    We need to make sure that movw is only used for architectures which satisfy arm_arch_thumb2 as indicated in the following predicate added:
>
> +(define_predicate "arm_movw_immediate_operand"
> +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> +       (and (match_code "const_int")
> +	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
>
>    I am modifying the predicate in order to fix issue for other older architectures.
>    It seems we can't achieve this by simply using 'n' instead of 'j' here, right?
>    Thanks.
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-18 12:04         ` Ramana Radhakrishnan
@ 2014-11-18 12:47           ` Yangfei (Felix)
  2014-11-18 12:56             ` Ramana Radhakrishnan
  0 siblings, 1 reply; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-18 12:47 UTC (permalink / raw)
  To: Ramana Radhakrishnan, gcc-patches, nickc, paul, Richard Earnshaw
  Cc: Chenshanyao

> On 18/11/14 11:02, Yangfei (Felix) wrote:
> >> On 06/11/14 08:35, Yangfei (Felix) wrote:
> >>>>>        The idea is simple: Use movw for certain const source
> >>>>> operand instead of
> >>>> ldrh.  And exclude the const values which cannot be handled by
> >>>> mov/mvn/movw.
> >>>>>        I am doing regression test for this patch.  Assuming no
> >>>>> issue pops up,
> >>>> OK for trunk?
> >>>>
> >>>> So, doesn't that makes the bug latent for architectures older than
> >>>> armv6t2 and big endian and only fixed this in ARM state ? I'd
> >>>> prefer a complete solution please. What about *thumb2_movhi_insn in
> >> thumb2.md ?
> >>>>
> >>>
> >>> Actually, we fix the bug by excluding the const values which cannot be
> handled.
> >> The patch still works even without the adding of "movw" here.
> >>> The new "movw" alternative here is just an small code optimization
> >>> for certain
> >> arch. We can handle consts by movw instead of ldrh and this better
> >> for performance.
> >>> We find that this bug is not reproducible for *thumb2_movhi_insn.
> >>> The reason
> >> is that this pattern can always move consts using "movw".
> >>
> >> Please fix the PR number in your final commit with PR 59593.
> >>
> >>> Index: gcc/config/arm/predicates.md
> >>>
> >>
> =============================================================
> >> ======
> >>> --- gcc/config/arm/predicates.md	(revision 216838)
> >>> +++ gcc/config/arm/predicates.md	(working copy)
> >>> @@ -144,6 +144,11 @@
> >>>     (and (match_code "const_int")
> >>>          (match_test "INTVAL (op) == 0")))
> >>>
> >>> +(define_predicate "arm_movw_immediate_operand"
> >>> +  (and (match_test "TARGET_32BIT && arm_arch_thumb2")
> >>> +       (and (match_code "const_int")
> >>> +	    (match_test "(INTVAL (op) & 0xffff0000) == 0"))))
> >>> +
> >>>   ;; Something valid on the RHS of an ARM data-processing
> >>> instruction (define_predicate "arm_rhs_operand"
> >>>     (ior (match_operand 0 "s_register_operand") @@ -211,6 +216,11 @@
> >>>     (ior (match_operand 0 "arm_rhs_operand")
> >>>          (match_operand 0 "arm_not_immediate_operand")))
> >>>
> >>> +(define_predicate "arm_hi_operand"
> >>> +  (ior (match_operand 0 "arm_rhsm_operand")
> >>> +       (ior (match_operand 0 "arm_not_immediate_operand")
> >>> +            (match_operand 0 "arm_movw_immediate_operand"))))
> >>> +
> >>
> >> I think you don't need any of these predicates.
> >>
> >>
> >>>   (define_predicate "arm_di_operand"
> >>>     (ior (match_operand 0 "s_register_operand")
> >>>          (match_operand 0 "arm_immediate_di_operand")))
> >>> Index: gcc/config/arm/arm.md
> >>>
> >>
> =============================================================
> >> ======
> >>> --- gcc/config/arm/arm.md	(revision 216838)
> >>> +++ gcc/config/arm/arm.md	(working copy)
> >>> @@ -6285,8 +6285,8 @@
> >>>
> >>>   ;; Pattern to recognize insn generated default case above
> >>> (define_insn "*movhi_insn_arch4"
> >>> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
> >>> -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
> >>> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
> >>> +	(match_operand:HI 1 "arm_hi_operand" "rIk,K,j,r,mi"))]
> >>
> >> Use `n' instead of `j' - movw can handle all of the numerical
> >> constants for HImode values. And the predicate can remain general_operand.
> >>
> 
> Did you read my comment about set_attr "arch" further down in the thread ?
> 
> > Look at the set_attr "arch" alternative and set this to t2 for the movw
> alternative. I would expect that to be enough to sort this out instead of adding all
> this code.
> 

Sorry for missing the point.  It seems to me that 't2' here will conflict with condition of the pattern *movhi_insn_arch4: 
  "TARGET_ARM
   && arm_arch4
   && (register_operand (operands[0], HImode)
       || register_operand (operands[1], HImode))"

#define TARGET_ARM                      (! TARGET_THUMB)
/* 32-bit Thumb-2 code.  */
#define TARGET_THUMB2                   (TARGET_THUMB && arm_arch_thumb2)

Right? Thanks.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-18 12:47           ` Yangfei (Felix)
@ 2014-11-18 12:56             ` Ramana Radhakrishnan
  2014-11-19  9:42               ` Yangfei (Felix)
  0 siblings, 1 reply; 17+ messages in thread
From: Ramana Radhakrishnan @ 2014-11-18 12:56 UTC (permalink / raw)
  To: Yangfei (Felix), gcc-patches, nickc, paul, Richard Earnshaw; +Cc: Chenshanyao


>
> Sorry for missing the point.  It seems to me that 't2' here will conflict with condition of the pattern *movhi_insn_arch4:
>    "TARGET_ARM
>     && arm_arch4
>     && (register_operand (operands[0], HImode)
>         || register_operand (operands[1], HImode))"
>
> #define TARGET_ARM                      (! TARGET_THUMB)
> /* 32-bit Thumb-2 code.  */
> #define TARGET_THUMB2                   (TARGET_THUMB && arm_arch_thumb2)
>

Bah, Indeed ! - I misremembered the t2 there, my mistake.

Yes you are right there, but what I'd like you to do is to use that 
mechanism rather than putting all this logic in the predicate.

So, I'd prefer you to add a v6t2 to the values for the "arch" attribute, 
don't forget to update the comments above.

and in arch_enabled you need to enforce this with

  (and (eq_attr "arch" "v6t2")
       (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
	 (const_string "yes")

And in the pattern use v6t2 ...

arm_arch_thumb2 implies that this is at the architecture level of v6t2. 
Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state.



regards
Ramana





> Right? Thanks.
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-18 12:56             ` Ramana Radhakrishnan
@ 2014-11-19  9:42               ` Yangfei (Felix)
  2014-11-19 10:26                 ` Ramana Radhakrishnan
  2014-11-20  9:24                 ` Ramana Radhakrishnan
  0 siblings, 2 replies; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-19  9:42 UTC (permalink / raw)
  To: gcc-patches, Ramana Radhakrishnan; +Cc: Chenshanyao

[-- Attachment #1: Type: text/plain, Size: 4933 bytes --]

> > Sorry for missing the point.  It seems to me that 't2' here will conflict with
> condition of the pattern *movhi_insn_arch4:
> >    "TARGET_ARM
> >     && arm_arch4
> >     && (register_operand (operands[0], HImode)
> >         || register_operand (operands[1], HImode))"
> >
> > #define TARGET_ARM                      (! TARGET_THUMB)
> > /* 32-bit Thumb-2 code.  */
> > #define TARGET_THUMB2                   (TARGET_THUMB &&
> arm_arch_thumb2)
> >
> 
> Bah, Indeed ! - I misremembered the t2 there, my mistake.
> 
> Yes you are right there, but what I'd like you to do is to use that mechanism
> rather than putting all this logic in the predicate.
> 
> So, I'd prefer you to add a v6t2 to the values for the "arch" attribute, don't forget
> to update the comments above.
> 
> and in arch_enabled you need to enforce this with
> 
>   (and (eq_attr "arch" "v6t2")
>        (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
> 	 (const_string "yes")
> 
> And in the pattern use v6t2 ...
> 
> arm_arch_thumb2 implies that this is at the architecture level of v6t2.
> Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state.


Hi Ramana, 
    Thank you for your suggestions.  I rebased the patch on the latest trunk and updated it accordingly. 
    As this patch will not work for architectures older than armv6t2,  I also prefer Thomas's patch to fix for them. 
    I am currently performing test for this patch.  Assuming no issues pops up, OK for the trunk?  
    And is it necessary to backport this patch to the 4.8 & 4.9 branches? 


Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 217717)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2014-11-19  Felix Yang  <felix.yang@huawei.com>
+	    Shanyao Chen  <chenshanyao@huawei.com>
+
+	PR target/59593
+	* config/arm/arm.md (define_attr "arch"): Add v6t2.
+	(define_attr "arch_enabled"): Add test for the above.
+	(*movhi_insn_arch4): Add new alternative.
+
 2014-11-18  Felix Yang  <felix.yang@huawei.com>
 
 	* config/aarch64/aarch64.c (doloop_end): New pattern.
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 217717)
+++ gcc/config/arm/arm.md	(working copy)
@@ -125,9 +125,10 @@
 ; This can be "a" for ARM, "t" for either of the Thumbs, "32" for
 ; TARGET_32BIT, "t1" or "t2" to specify a specific Thumb mode.  "v6"
 ; for ARM or Thumb-2 with arm_arch6, and nov6 for ARM without
-; arm_arch6.  This attribute is used to compute attribute "enabled",
-; use type "any" to enable an alternative in all cases.
-(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
+; arm_arch6.  "v6t2" for Thumb-2 with arm_arch6.  This attribute is
+; used to compute attribute "enabled", use type "any" to enable an
+; alternative in all cases.
+(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,v6t2,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
   (const_string "any"))
 
 (define_attr "arch_enabled" "no,yes"
@@ -162,6 +163,10 @@
 	      (match_test "TARGET_32BIT && !arm_arch6"))
 	 (const_string "yes")
 
+	 (and (eq_attr "arch" "v6t2")
+	      (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
+	 (const_string "yes")
+
 	 (and (eq_attr "arch" "avoid_neon_for_64bits")
 	      (match_test "TARGET_NEON")
 	      (not (match_test "TARGET_PREFER_NEON_64BITS")))
@@ -6288,8 +6293,8 @@
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
-	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
+	(match_operand:HI 1 "general_operand"      "rIk,K,n,r,mi"))]
   "TARGET_ARM
    && arm_arch4
    && (register_operand (operands[0], HImode)
@@ -6297,16 +6302,19 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
+   movw%?\\t%0, %1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,256")
-   (set_attr "neg_pool_range" "*,*,*,244")
+   (set_attr "pool_range" "*,*,*,*,256")
+   (set_attr "neg_pool_range" "*,*,*,*,244")
+   (set_attr "arch" "*,*,v6t2,*,*")
    (set_attr_alternative "type"
                          [(if_then_else (match_operand 1 "const_int_operand" "")
                                         (const_string "mov_imm" )
                                         (const_string "mov_reg"))
                           (const_string "mvn_imm")
+                          (const_string "mov_imm")
                           (const_string "store1")
                           (const_string "load1")])]
 )

[-- Attachment #2: arm-patch-v3.diff --]
[-- Type: application/octet-stream, Size: 3258 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 217717)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2014-11-19  Felix Yang  <felix.yang@huawei.com>
+	    Shanyao Chen  <chenshanyao@huawei.com>
+
+	PR target/59593
+	* config/arm/arm.md (define_attr "arch"): Add v6t2.
+	(define_attr "arch_enabled"): Add test for the above.
+	(*movhi_insn_arch4): Add new alternative.
+
 2014-11-18  Felix Yang  <felix.yang@huawei.com>
 
 	* config/aarch64/aarch64.c (doloop_end): New pattern.
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 217717)
+++ gcc/config/arm/arm.md	(working copy)
@@ -125,9 +125,10 @@
 ; This can be "a" for ARM, "t" for either of the Thumbs, "32" for
 ; TARGET_32BIT, "t1" or "t2" to specify a specific Thumb mode.  "v6"
 ; for ARM or Thumb-2 with arm_arch6, and nov6 for ARM without
-; arm_arch6.  This attribute is used to compute attribute "enabled",
-; use type "any" to enable an alternative in all cases.
-(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
+; arm_arch6.  "v6t2" for Thumb-2 with arm_arch6.  This attribute is
+; used to compute attribute "enabled", use type "any" to enable an
+; alternative in all cases.
+(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,v6t2,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
   (const_string "any"))
 
 (define_attr "arch_enabled" "no,yes"
@@ -162,6 +163,10 @@
 	      (match_test "TARGET_32BIT && !arm_arch6"))
 	 (const_string "yes")
 
+	 (and (eq_attr "arch" "v6t2")
+	      (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
+	 (const_string "yes")
+
 	 (and (eq_attr "arch" "avoid_neon_for_64bits")
 	      (match_test "TARGET_NEON")
 	      (not (match_test "TARGET_PREFER_NEON_64BITS")))
@@ -6288,8 +6293,8 @@
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
-	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
+	(match_operand:HI 1 "general_operand"      "rIk,K,n,r,mi"))]
   "TARGET_ARM
    && arm_arch4
    && (register_operand (operands[0], HImode)
@@ -6297,16 +6302,19 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
+   movw%?\\t%0, %1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,256")
-   (set_attr "neg_pool_range" "*,*,*,244")
+   (set_attr "pool_range" "*,*,*,*,256")
+   (set_attr "neg_pool_range" "*,*,*,*,244")
+   (set_attr "arch" "*,*,v6t2,*,*")
    (set_attr_alternative "type"
                          [(if_then_else (match_operand 1 "const_int_operand" "")
                                         (const_string "mov_imm" )
                                         (const_string "mov_reg"))
                           (const_string "mvn_imm")
+                          (const_string "mov_imm")
                           (const_string "store1")
                           (const_string "load1")])]
 )

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-19  9:42               ` Yangfei (Felix)
@ 2014-11-19 10:26                 ` Ramana Radhakrishnan
  2014-11-20  9:24                 ` Ramana Radhakrishnan
  1 sibling, 0 replies; 17+ messages in thread
From: Ramana Radhakrishnan @ 2014-11-19 10:26 UTC (permalink / raw)
  To: Yangfei (Felix), gcc-patches; +Cc: Chenshanyao



On 19/11/14 09:29, Yangfei (Felix) wrote:
>>> Sorry for missing the point.  It seems to me that 't2' here will conflict with
>> condition of the pattern *movhi_insn_arch4:
>>>     "TARGET_ARM
>>>      && arm_arch4
>>>      && (register_operand (operands[0], HImode)
>>>          || register_operand (operands[1], HImode))"
>>>
>>> #define TARGET_ARM                      (! TARGET_THUMB)
>>> /* 32-bit Thumb-2 code.  */
>>> #define TARGET_THUMB2                   (TARGET_THUMB &&
>> arm_arch_thumb2)
>>>
>>
>> Bah, Indeed ! - I misremembered the t2 there, my mistake.
>>
>> Yes you are right there, but what I'd like you to do is to use that mechanism
>> rather than putting all this logic in the predicate.
>>
>> So, I'd prefer you to add a v6t2 to the values for the "arch" attribute, don't forget
>> to update the comments above.
>>
>> and in arch_enabled you need to enforce this with
>>
>>    (and (eq_attr "arch" "v6t2")
>>         (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
>> 	 (const_string "yes")
>>
>> And in the pattern use v6t2 ...
>>
>> arm_arch_thumb2 implies that this is at the architecture level of v6t2.
>> Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state.
>
>
> Hi Ramana,
>      Thank you for your suggestions.  I rebased the patch on the latest trunk and updated it accordingly.
>      As this patch will not work for architectures older than armv6t2,  I also prefer Thomas's patch to fix for them.
>      I am currently performing test for this patch.  Assuming no issues pops up, OK for the trunk?
>      And is it necessary to backport this patch to the 4.8 & 4.9 branches?



This is OK for trunk if no regressions - please let it bake for a few 
days on trunk to let auto-testers catch up. This is OK for 4.8 / 4.9 
after that and please test your backport.

regards
Ramana


>
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog	(revision 217717)
> +++ gcc/ChangeLog	(working copy)
> @@ -1,3 +1,11 @@
> +2014-11-19  Felix Yang  <felix.yang@huawei.com>
> +	    Shanyao Chen  <chenshanyao@huawei.com>
> +
> +	PR target/59593
> +	* config/arm/arm.md (define_attr "arch"): Add v6t2.
> +	(define_attr "arch_enabled"): Add test for the above.
> +	(*movhi_insn_arch4): Add new alternative.
> +
>   2014-11-18  Felix Yang  <felix.yang@huawei.com>
>
>   	* config/aarch64/aarch64.c (doloop_end): New pattern.
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md	(revision 217717)
> +++ gcc/config/arm/arm.md	(working copy)
> @@ -125,9 +125,10 @@
>   ; This can be "a" for ARM, "t" for either of the Thumbs, "32" for
>   ; TARGET_32BIT, "t1" or "t2" to specify a specific Thumb mode.  "v6"
>   ; for ARM or Thumb-2 with arm_arch6, and nov6 for ARM without
> -; arm_arch6.  This attribute is used to compute attribute "enabled",
> -; use type "any" to enable an alternative in all cases.
> -(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
> +; arm_arch6.  "v6t2" for Thumb-2 with arm_arch6.  This attribute is
> +; used to compute attribute "enabled", use type "any" to enable an
> +; alternative in all cases.
> +(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,v6t2,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
>     (const_string "any"))
>
>   (define_attr "arch_enabled" "no,yes"
> @@ -162,6 +163,10 @@
>   	      (match_test "TARGET_32BIT && !arm_arch6"))
>   	 (const_string "yes")
>
> +	 (and (eq_attr "arch" "v6t2")
> +	      (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
> +	 (const_string "yes")
> +
>   	 (and (eq_attr "arch" "avoid_neon_for_64bits")
>   	      (match_test "TARGET_NEON")
>   	      (not (match_test "TARGET_PREFER_NEON_64BITS")))
> @@ -6288,8 +6293,8 @@
>
>   ;; Pattern to recognize insn generated default case above
>   (define_insn "*movhi_insn_arch4"
> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
> -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
> +	(match_operand:HI 1 "general_operand"      "rIk,K,n,r,mi"))]
>     "TARGET_ARM
>      && arm_arch4
>      && (register_operand (operands[0], HImode)
> @@ -6297,16 +6302,19 @@
>     "@
>      mov%?\\t%0, %1\\t%@ movhi
>      mvn%?\\t%0, #%B1\\t%@ movhi
> +   movw%?\\t%0, %1\\t%@ movhi
>      str%(h%)\\t%1, %0\\t%@ movhi
>      ldr%(h%)\\t%0, %1\\t%@ movhi"
>     [(set_attr "predicable" "yes")
> -   (set_attr "pool_range" "*,*,*,256")
> -   (set_attr "neg_pool_range" "*,*,*,244")
> +   (set_attr "pool_range" "*,*,*,*,256")
> +   (set_attr "neg_pool_range" "*,*,*,*,244")
> +   (set_attr "arch" "*,*,v6t2,*,*")
>      (set_attr_alternative "type"
>                            [(if_then_else (match_operand 1 "const_int_operand" "")
>                                           (const_string "mov_imm" )
>                                           (const_string "mov_reg"))
>                             (const_string "mvn_imm")
> +                          (const_string "mov_imm")
>                             (const_string "store1")
>                             (const_string "load1")])]
>   )
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-19  9:42               ` Yangfei (Felix)
  2014-11-19 10:26                 ` Ramana Radhakrishnan
@ 2014-11-20  9:24                 ` Ramana Radhakrishnan
  2014-11-20  9:25                   ` Yangfei (Felix)
  2014-11-20  9:48                   ` Yangfei (Felix)
  1 sibling, 2 replies; 17+ messages in thread
From: Ramana Radhakrishnan @ 2014-11-20  9:24 UTC (permalink / raw)
  To: Yangfei (Felix), gcc-patches; +Cc: Chenshanyao, Kugan

[-- Attachment #1: Type: text/plain, Size: 5552 bytes --]



On 19/11/14 09:29, Yangfei (Felix) wrote:
>>> Sorry for missing the point.  It seems to me that 't2' here will conflict with
>> condition of the pattern *movhi_insn_arch4:
>>>     "TARGET_ARM
>>>      && arm_arch4
>>>      && (register_operand (operands[0], HImode)
>>>          || register_operand (operands[1], HImode))"
>>>
>>> #define TARGET_ARM                      (! TARGET_THUMB)
>>> /* 32-bit Thumb-2 code.  */
>>> #define TARGET_THUMB2                   (TARGET_THUMB &&
>> arm_arch_thumb2)
>>>
>>
>> Bah, Indeed ! - I misremembered the t2 there, my mistake.
>>
>> Yes you are right there, but what I'd like you to do is to use that mechanism
>> rather than putting all this logic in the predicate.
>>
>> So, I'd prefer you to add a v6t2 to the values for the "arch" attribute, don't forget
>> to update the comments above.
>>
>> and in arch_enabled you need to enforce this with
>>
>>    (and (eq_attr "arch" "v6t2")
>>         (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
>> 	 (const_string "yes")
>>
>> And in the pattern use v6t2 ...
>>
>> arm_arch_thumb2 implies that this is at the architecture level of v6t2.
>> Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state.
>
>
> Hi Ramana,
>      Thank you for your suggestions.  I rebased the patch on the latest trunk and updated it accordingly.
>      As this patch will not work for architectures older than armv6t2,  I also prefer Thomas's patch to fix for them.
>      I am currently performing test for this patch.  Assuming no issues pops up, OK for the trunk?
>      And is it necessary to backport this patch to the 4.8 & 4.9 branches?
>

I've applied the following as obvious after Kugan mentioned on IRC this 
morning noticing a movwne r0, #-32768. Obviously this won't be accepted 
as is by the assembler and we should be using the %L character. Applied 
to trunk as obvious.

Felix, How did you test this patch ?

regards
Ramana

2014-11-20  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

         PR target/59593
         * config/arm/arm.md (*movhi_insn): Use right formatting
         for immediate.




>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog	(revision 217717)
> +++ gcc/ChangeLog	(working copy)
> @@ -1,3 +1,11 @@
> +2014-11-19  Felix Yang  <felix.yang@huawei.com>
> +	    Shanyao Chen  <chenshanyao@huawei.com>
> +
> +	PR target/59593
> +	* config/arm/arm.md (define_attr "arch"): Add v6t2.
> +	(define_attr "arch_enabled"): Add test for the above.
> +	(*movhi_insn_arch4): Add new alternative.
> +
>   2014-11-18  Felix Yang  <felix.yang@huawei.com>
>
>   	* config/aarch64/aarch64.c (doloop_end): New pattern.
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md	(revision 217717)
> +++ gcc/config/arm/arm.md	(working copy)
> @@ -125,9 +125,10 @@
>   ; This can be "a" for ARM, "t" for either of the Thumbs, "32" for
>   ; TARGET_32BIT, "t1" or "t2" to specify a specific Thumb mode.  "v6"
>   ; for ARM or Thumb-2 with arm_arch6, and nov6 for ARM without
> -; arm_arch6.  This attribute is used to compute attribute "enabled",
> -; use type "any" to enable an alternative in all cases.
> -(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
> +; arm_arch6.  "v6t2" for Thumb-2 with arm_arch6.  This attribute is
> +; used to compute attribute "enabled", use type "any" to enable an
> +; alternative in all cases.
> +(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,v6t2,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
>     (const_string "any"))
>
>   (define_attr "arch_enabled" "no,yes"
> @@ -162,6 +163,10 @@
>   	      (match_test "TARGET_32BIT && !arm_arch6"))
>   	 (const_string "yes")
>
> +	 (and (eq_attr "arch" "v6t2")
> +	      (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
> +	 (const_string "yes")
> +
>   	 (and (eq_attr "arch" "avoid_neon_for_64bits")
>   	      (match_test "TARGET_NEON")
>   	      (not (match_test "TARGET_PREFER_NEON_64BITS")))
> @@ -6288,8 +6293,8 @@
>
>   ;; Pattern to recognize insn generated default case above
>   (define_insn "*movhi_insn_arch4"
> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
> -	(match_operand:HI 1 "general_operand"      "rIk,K,r,mi"))]
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
> +	(match_operand:HI 1 "general_operand"      "rIk,K,n,r,mi"))]
>     "TARGET_ARM
>      && arm_arch4
>      && (register_operand (operands[0], HImode)
> @@ -6297,16 +6302,19 @@
>     "@
>      mov%?\\t%0, %1\\t%@ movhi
>      mvn%?\\t%0, #%B1\\t%@ movhi
> +   movw%?\\t%0, %1\\t%@ movhi
>      str%(h%)\\t%1, %0\\t%@ movhi
>      ldr%(h%)\\t%0, %1\\t%@ movhi"
>     [(set_attr "predicable" "yes")
> -   (set_attr "pool_range" "*,*,*,256")
> -   (set_attr "neg_pool_range" "*,*,*,244")
> +   (set_attr "pool_range" "*,*,*,*,256")
> +   (set_attr "neg_pool_range" "*,*,*,*,244")
> +   (set_attr "arch" "*,*,v6t2,*,*")
>      (set_attr_alternative "type"
>                            [(if_then_else (match_operand 1 "const_int_operand" "")
>                                           (const_string "mov_imm" )
>                                           (const_string "mov_reg"))
>                             (const_string "mvn_imm")
> +                          (const_string "mov_imm")
>                             (const_string "store1")
>                             (const_string "load1")])]
>   )
>

[-- Attachment #2: p5.txt --]
[-- Type: text/plain, Size: 399 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 3a6e0b0..a52716d 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -6302,7 +6302,7 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
-   movw%?\\t%0, %1\\t%@ movhi
+   movw%?\\t%0, %L1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-20  9:24                 ` Ramana Radhakrishnan
@ 2014-11-20  9:25                   ` Yangfei (Felix)
  2014-11-20  9:48                   ` Yangfei (Felix)
  1 sibling, 0 replies; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-20  9:25 UTC (permalink / raw)
  To: Ramana Radhakrishnan, gcc-patches; +Cc: Chenshanyao, Kugan

> On 19/11/14 09:29, Yangfei (Felix) wrote:
> >>> Sorry for missing the point.  It seems to me that 't2' here will
> >>> conflict with
> >> condition of the pattern *movhi_insn_arch4:
> >>>     "TARGET_ARM
> >>>      && arm_arch4
> >>>      && (register_operand (operands[0], HImode)
> >>>          || register_operand (operands[1], HImode))"
> >>>
> >>> #define TARGET_ARM                      (! TARGET_THUMB)
> >>> /* 32-bit Thumb-2 code.  */
> >>> #define TARGET_THUMB2                   (TARGET_THUMB &&
> >> arm_arch_thumb2)
> >>>
> >>
> >> Bah, Indeed ! - I misremembered the t2 there, my mistake.
> >>
> >> Yes you are right there, but what I'd like you to do is to use that
> >> mechanism rather than putting all this logic in the predicate.
> >>
> >> So, I'd prefer you to add a v6t2 to the values for the "arch"
> >> attribute, don't forget to update the comments above.
> >>
> >> and in arch_enabled you need to enforce this with
> >>
> >>    (and (eq_attr "arch" "v6t2")
> >>         (match_test "TARGET_32BIT && arm_arch6 &&
> arm_arch_thumb2"))
> >> 	 (const_string "yes")
> >>
> >> And in the pattern use v6t2 ...
> >>
> >> arm_arch_thumb2 implies that this is at the architecture level of v6t2.
> >> Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state.
> >
> >
> > Hi Ramana,
> >      Thank you for your suggestions.  I rebased the patch on the latest trunk
> and updated it accordingly.
> >      As this patch will not work for architectures older than armv6t2,  I also
> prefer Thomas's patch to fix for them.
> >      I am currently performing test for this patch.  Assuming no issues pops
> up, OK for the trunk?
> >      And is it necessary to backport this patch to the 4.8 & 4.9 branches?
> >
> 
> I've applied the following as obvious after Kugan mentioned on IRC this morning
> noticing a movwne r0, #-32768. Obviously this won't be accepted as is by the
> assembler and we should be using the %L character. Applied to trunk as obvious.
> 
> Felix, How did you test this patch ?
> 
> regards
> Ramana


I regtested the patch for arm-eabi-gcc/g++ & big-endian with qemu.  The test result is OK.  That's strange ...  

This issue can be reproduced by the following testcase.  Thanks for fixing it.  

#include <stdio.h>
unsigned short v = 0x5678;
int i;
int j = 0;
int *ptr = &j;
int func()
{
        for (i = 0; i < 1; ++i)
        {
                *ptr = -1;
                v = 0xF234;
        }
        return v;
}

> 
> 2014-11-20  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>
> 
>          PR target/59593
>          * config/arm/arm.md (*movhi_insn): Use right formatting
>          for immediate.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian
  2014-11-20  9:24                 ` Ramana Radhakrishnan
  2014-11-20  9:25                   ` Yangfei (Felix)
@ 2014-11-20  9:48                   ` Yangfei (Felix)
  2014-11-29 10:58                     ` [PATCH PR59593] [arm] Backport r217772 & r217826 to 4.8 & 4.9 Chen Shanyao
  1 sibling, 1 reply; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-20  9:48 UTC (permalink / raw)
  To: Ramana Radhakrishnan, gcc-patches; +Cc: Chenshanyao, Kugan

> > On 19/11/14 09:29, Yangfei (Felix) wrote:
> > >>> Sorry for missing the point.  It seems to me that 't2' here will
> > >>> conflict with
> > >> condition of the pattern *movhi_insn_arch4:
> > >>>     "TARGET_ARM
> > >>>      && arm_arch4
> > >>>      && (register_operand (operands[0], HImode)
> > >>>          || register_operand (operands[1], HImode))"
> > >>>
> > >>> #define TARGET_ARM                      (! TARGET_THUMB)
> > >>> /* 32-bit Thumb-2 code.  */
> > >>> #define TARGET_THUMB2                   (TARGET_THUMB &&
> > >> arm_arch_thumb2)
> > >>>
> > >>
> > >> Bah, Indeed ! - I misremembered the t2 there, my mistake.
> > >>
> > >> Yes you are right there, but what I'd like you to do is to use that
> > >> mechanism rather than putting all this logic in the predicate.
> > >>
> > >> So, I'd prefer you to add a v6t2 to the values for the "arch"
> > >> attribute, don't forget to update the comments above.
> > >>
> > >> and in arch_enabled you need to enforce this with
> > >>
> > >>    (and (eq_attr "arch" "v6t2")
> > >>         (match_test "TARGET_32BIT && arm_arch6 &&
> > arm_arch_thumb2"))
> > >> 	 (const_string "yes")
> > >>
> > >> And in the pattern use v6t2 ...
> > >>
> > >> arm_arch_thumb2 implies that this is at the architecture level of v6t2.
> > >> Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state.
> > >
> > >
> > > Hi Ramana,
> > >      Thank you for your suggestions.  I rebased the patch on the
> > > latest trunk
> > and updated it accordingly.
> > >      As this patch will not work for architectures older than
> > > armv6t2,  I also
> > prefer Thomas's patch to fix for them.
> > >      I am currently performing test for this patch.  Assuming no
> > > issues pops
> > up, OK for the trunk?
> > >      And is it necessary to backport this patch to the 4.8 & 4.9 branches?
> > >
> >
> > I've applied the following as obvious after Kugan mentioned on IRC
> > this morning noticing a movwne r0, #-32768. Obviously this won't be
> > accepted as is by the assembler and we should be using the %L character.
> Applied to trunk as obvious.
> >
> > Felix, How did you test this patch ?
> >
> > regards
> > Ramana
> 
> 
> I regtested the patch for arm-eabi-gcc/g++ & big-endian with qemu.  The test
> result is OK.  That's strange ...
> 
> This issue can be reproduced by the following testcase.  Thanks for fixing it.
> 
> #include <stdio.h>
> unsigned short v = 0x5678;
> int i;
> int j = 0;
> int *ptr = &j;
> int func()
> {
>         for (i = 0; i < 1; ++i)
>         {
>                 *ptr = -1;
>                 v = 0xF234;
>         }
>         return v;
> }
>

And the architecture level is set to armv7-a by default when testing. 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH PR59593] [arm] Backport r217772 & r217826  to 4.8 & 4.9
  2014-11-20  9:48                   ` Yangfei (Felix)
@ 2014-11-29 10:58                     ` Chen Shanyao
  2014-11-29 12:11                       ` Yangfei (Felix)
  2014-12-02 12:22                       ` Ramana Radhakrishnan
  0 siblings, 2 replies; 17+ messages in thread
From: Chen Shanyao @ 2014-11-29 10:58 UTC (permalink / raw)
  To: Yangfei (Felix), Ramana Radhakrishnan, gcc-patches; +Cc: Kugan

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

I've backported this fix to 4.8 & 4.9 branch.
These patches have been tested for armeb-none-eabi-gcc/g++ with qemu, 
and both the test results were ok.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: backport-PR59593-4.8_v1.patch --]
[-- Type: text/plain; charset="gb18030"; name="backport-PR59593-4.8_v1.patch", Size: 3313 bytes --]

--- gcc/ChangeLog.ori	2014-11-28 17:30:43.000000000 +0800
+++ gcc/ChangeLog	2014-11-28 17:30:55.000000000 +0800
@@ -1,3 +1,15 @@
+2014-11-28  Felix Yang  <felix.yang@huawei.com>
+           Shanyao Chen  <chenshanyao@huawei.com>
+
+        Backport from mainline
+        2014-11-19  Felix Yang  <felix.yang@huawei.com>
+                    Shanyao Chen  <chenshanyao@huawei.com>
+
+        PR target/59593
+        * config/arm/arm.md (define_attr "arch"): Add v6t2.
+        (define_attr "arch_enabled"): Add test for the above.
+        (*movhi_insn_arch4): Add new alternative.
+
 2014-11-19  Uros Bizjak  <ubizjak@gmail.com>
 
 	PR target/63947
--- gcc/config/arm/arm.md.ori	2014-11-28 17:33:12.000000000 +0800
+++ gcc/config/arm/arm.md	2014-11-29 10:34:28.000000000 +0800
@@ -92,9 +92,11 @@
 ; This can be "a" for ARM, "t" for either of the Thumbs, "32" for
 ; TARGET_32BIT, "t1" or "t2" to specify a specific Thumb mode.  "v6"
 ; for ARM or Thumb-2 with arm_arch6, and nov6 for ARM without
-; arm_arch6.  This attribute is used to compute attribute "enabled",
-; use type "any" to enable an alternative in all cases.
-(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,onlya8,neon_onlya8,nota8,neon_nota8,iwmmxt,iwmmxt2"
+; arm_arch6.  "v6t2" for Thumb-2 with arm_arch6.  This attribute is
+; used to compute attribute "enabled", use type "any" to enable an
+; alternative in all cases.
+
+(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,v6t2,onlya8,neon_onlya8,nota8,neon_nota8,iwmmxt,iwmmxt2"
   (const_string "any"))
 
 (define_attr "arch_enabled" "no,yes"
@@ -129,6 +131,10 @@
 	      (match_test "TARGET_32BIT && !arm_arch6"))
 	 (const_string "yes")
 
+	 (and (eq_attr "arch" "v6t2")
+	      (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
+	 (const_string "yes")
+
 	 (and (eq_attr "arch" "onlya8")
 	      (eq_attr "tune" "cortexa8"))
 	 (const_string "yes")
@@ -6282,8 +6288,8 @@
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
-	(match_operand:HI 1 "general_operand"      "rI,K,r,mi"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
+	(match_operand:HI 1 "general_operand"      "rI,K,n,r,mi"))]
   "TARGET_ARM
    && arm_arch4
    && (register_operand (operands[0], HImode)
@@ -6291,17 +6297,20 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
+   movw%?\\t%0, %L1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")
-   (set_attr "insn" "mov,mvn,*,*")
-   (set_attr "pool_range" "*,*,*,256")
-   (set_attr "neg_pool_range" "*,*,*,244")
+   (set_attr "insn" "mov,mvn,mov,*,*")
+   (set_attr "pool_range" "*,*,*,*,256")
+   (set_attr "neg_pool_range" "*,*,*,*,244")
+   (set_attr "arch" "*,*,v6t2,*,*")
    (set_attr_alternative "type"
                          [(if_then_else (match_operand 1 "const_int_operand" "")
                                         (const_string "simple_alu_imm" )
                                         (const_string "*"))
                           (const_string "simple_alu_imm")
+                          (const_string "simple_alu_imm")
                           (const_string "store1")
                           (const_string "load1")])]
 )

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: backport-PR59593-4.9_v1.patch --]
[-- Type: text/plain; charset="gb18030"; name="backport-PR59593-4.9_v1.patch", Size: 3278 bytes --]

--- gcc/ChangeLog.ori	2014-11-28 17:20:38.000000000 +0800
+++ gcc/ChangeLog	2014-11-28 17:21:14.000000000 +0800
@@ -1,3 +1,15 @@
+2014-11-28  Felix Yang  <felix.yang@huawei.com>
+           Shanyao Chen  <chenshanyao@huawei.com>
+
+        Backport from mainline
+        2014-11-19  Felix Yang  <felix.yang@huawei.com>
+                    Shanyao Chen  <chenshanyao@huawei.com>
+
+        PR target/59593
+        * config/arm/arm.md (define_attr "arch"): Add v6t2.
+        (define_attr "arch_enabled"): Add test for the above.
+        (*movhi_insn_arch4): Add new alternative.
+
 2014-11-26  Richard Biener  <rguenther@suse.de>
 
 	PR middle-end/63738
--- gcc/config/arm/arm.md.ori	2014-11-28 09:34:21.000000000 +0800
+++ gcc/config/arm/arm.md	2014-11-29 10:28:13.000000000 +0800
@@ -125,9 +125,10 @@
 ; This can be "a" for ARM, "t" for either of the Thumbs, "32" for
 ; TARGET_32BIT, "t1" or "t2" to specify a specific Thumb mode.  "v6"
 ; for ARM or Thumb-2 with arm_arch6, and nov6 for ARM without
-; arm_arch6.  This attribute is used to compute attribute "enabled",
-; use type "any" to enable an alternative in all cases.
-(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2"
+; arm_arch6.  "v6t2" for Thumb-2 with arm_arch6.  This attribute is
+; used to compute attribute "enabled", use type "any" to enable an
+; alternative in all cases.
+(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,v6t2,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2"
   (const_string "any"))
 
 (define_attr "arch_enabled" "no,yes"
@@ -162,6 +163,10 @@
 	      (match_test "TARGET_32BIT && !arm_arch6"))
 	 (const_string "yes")
 
+	 (and (eq_attr "arch" "v6t2")
+	      (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
+	 (const_string "yes")
+
 	 (and (eq_attr "arch" "avoid_neon_for_64bits")
 	      (match_test "TARGET_NEON")
 	      (not (match_test "TARGET_PREFER_NEON_64BITS")))
@@ -6961,8 +6966,8 @@
 
 ;; Pattern to recognize insn generated default case above
 (define_insn "*movhi_insn_arch4"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r")
-	(match_operand:HI 1 "general_operand"      "rI,K,r,mi"))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,m,r")
+	(match_operand:HI 1 "general_operand"      "rI,K,n,r,mi"))]
   "TARGET_ARM
    && arm_arch4
    && (register_operand (operands[0], HImode)
@@ -6970,16 +6975,19 @@
   "@
    mov%?\\t%0, %1\\t%@ movhi
    mvn%?\\t%0, #%B1\\t%@ movhi
+   movw%?\\t%0, %L1\\t%@ movhi
    str%(h%)\\t%1, %0\\t%@ movhi
    ldr%(h%)\\t%0, %1\\t%@ movhi"
   [(set_attr "predicable" "yes")
-   (set_attr "pool_range" "*,*,*,256")
-   (set_attr "neg_pool_range" "*,*,*,244")
+   (set_attr "pool_range" "*,*,*,*,256")
+   (set_attr "neg_pool_range" "*,*,*,*,244")
+   (set_attr "arch" "*,*,v6t2,*,*")
    (set_attr_alternative "type"
                          [(if_then_else (match_operand 1 "const_int_operand" "")
                                         (const_string "mov_imm" )
                                         (const_string "mov_reg"))
                           (const_string "mvn_imm")
+                          (const_string "mov_imm")
                           (const_string "store1")
                           (const_string "load1")])]
 )

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH PR59593] [arm] Backport r217772 & r217826  to 4.8 & 4.9
  2014-11-29 10:58                     ` [PATCH PR59593] [arm] Backport r217772 & r217826 to 4.8 & 4.9 Chen Shanyao
@ 2014-11-29 12:11                       ` Yangfei (Felix)
  2014-12-02 12:22                       ` Ramana Radhakrishnan
  1 sibling, 0 replies; 17+ messages in thread
From: Yangfei (Felix) @ 2014-11-29 12:11 UTC (permalink / raw)
  To: Chenshanyao, Ramana Radhakrishnan, gcc-patches; +Cc: Kugan

> I've backported this fix to 4.8 & 4.9 branch.
> These patches have been tested for armeb-none-eabi-gcc/g++ with qemu, and
> both the test results were ok.

Looks OK with me.  Ramana, is this OK for the 4.8 & 4.9 branches?  Thanks. 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH PR59593] [arm] Backport r217772 & r217826  to 4.8 & 4.9
  2014-11-29 10:58                     ` [PATCH PR59593] [arm] Backport r217772 & r217826 to 4.8 & 4.9 Chen Shanyao
  2014-11-29 12:11                       ` Yangfei (Felix)
@ 2014-12-02 12:22                       ` Ramana Radhakrishnan
  1 sibling, 0 replies; 17+ messages in thread
From: Ramana Radhakrishnan @ 2014-12-02 12:22 UTC (permalink / raw)
  To: Chen Shanyao, Yangfei (Felix), gcc-patches; +Cc: Kugan, Richard Biener



On 29/11/14 06:50, Chen Shanyao wrote:
> I've backported this fix to 4.8 & 4.9 branch.
> These patches have been tested for armeb-none-eabi-gcc/g++ with qemu,
> and both the test results were ok.
>

The Changelog should mention all authors of the original patches i.e. 
include my name.

Otherwise this is OK unless an RM objects in the next 24 hours.



regards
Ramana

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2014-12-02 12:22 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-05  7:12 [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian Yangfei (Felix)
2014-11-05 11:01 ` Ramana Radhakrishnan
2014-11-06  8:36   ` Yangfei (Felix)
2014-11-18  8:54     ` Ramana Radhakrishnan
2014-11-18 11:36       ` Yangfei (Felix)
2014-11-18 12:04         ` Ramana Radhakrishnan
2014-11-18 12:47           ` Yangfei (Felix)
2014-11-18 12:56             ` Ramana Radhakrishnan
2014-11-19  9:42               ` Yangfei (Felix)
2014-11-19 10:26                 ` Ramana Radhakrishnan
2014-11-20  9:24                 ` Ramana Radhakrishnan
2014-11-20  9:25                   ` Yangfei (Felix)
2014-11-20  9:48                   ` Yangfei (Felix)
2014-11-29 10:58                     ` [PATCH PR59593] [arm] Backport r217772 & r217826 to 4.8 & 4.9 Chen Shanyao
2014-11-29 12:11                       ` Yangfei (Felix)
2014-12-02 12:22                       ` Ramana Radhakrishnan
2014-11-12 10:28   ` [PING][PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian Yangfei (Felix)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).