| Message ID | DA41BE1DDCA941489001C7FBD7A8820E5555144A@szxema507-mbx.china.huawei.com |
|---|---|
| State | New |
| Headers | show |
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
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")])]
> )
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")])] )
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?