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