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

["Yangfei (Felix)" <felix dot yang at huawei dot com>]

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