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