Re: [PATCH, PR63742][ARM] Fix arm *movhi_insn_arch4 pattern for big-endian

["Yangfei (Felix)" <felix.yang@huawei.com>]

> 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.