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

[Ramana Radhakrishnan <ramana dot radhakrishnan at arm dot com>]

On 19/11/14 09:29, Yangfei (Felix) wrote:
> > > 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)
> > >
> >
> > Bah, Indeed ! - I misremembered the t2 there, my mistake.
> >
> > Yes you are right there, but what I'd like you to do is to use that mechanism
> > rather than putting all this logic in the predicate.
> >
> > So, I'd prefer you to add a v6t2 to the values for the "arch" attribute, don't forget
> > to update the comments above.
> >
> > and in arch_enabled you need to enforce this with
> >
> >    (and (eq_attr "arch" "v6t2")
> >         (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
> > 	 (const_string "yes")
> >
> > And in the pattern use v6t2 ...
> >
> > arm_arch_thumb2 implies that this is at the architecture level of v6t2.
> > Therefore TARGET_ARM && arm_arch_thumb2 implies ARM state.
>
>
> Hi Ramana,
>      Thank you for your suggestions.  I rebased the patch on the latest trunk and updated it accordingly.
>      As this patch will not work for architectures older than armv6t2,  I also prefer Thomas's patch to fix for them.
>      I am currently performing test for this patch.  Assuming no issues pops up, OK for the trunk?
>      And is it necessary to backport this patch to the 4.8 & 4.9 branches?

I've applied the following as obvious after Kugan mentioned on IRC this 
morning noticing a movwne r0, #-32768. Obviously this won't be accepted 
as is by the assembler and we should be using the %L character. Applied 
to trunk as obvious.

Felix, How did you test this patch ?

regards
Ramana

2014-11-20  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

        PR target/59593
        * config/arm/arm.md (*movhi_insn): Use right formatting
        for immediate.




> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog	(revision 217717)
> +++ gcc/ChangeLog	(working copy)
> @@ -1,3 +1,11 @@
> +2014-11-19  Felix Yang  <felix.yang@huawei.com>
> +	    Shanyao Chen  <chenshanyao@huawei.com>
> +
> +	PR target/59593
> +	* config/arm/arm.md (define_attr "arch"): Add v6t2.
> +	(define_attr "arch_enabled"): Add test for the above.
> +	(*movhi_insn_arch4): Add new alternative.
> +
>   2014-11-18  Felix Yang  <felix.yang@huawei.com>
>
>   	* config/aarch64/aarch64.c (doloop_end): New pattern.
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md	(revision 217717)
> +++ gcc/config/arm/arm.md	(working copy)
> @@ -125,9 +125,10 @@
>   ; This can be "a" for ARM, "t" for either of the Thumbs, "32" for
>   ; TARGET_32BIT, "t1" or "t2" to specify a specific Thumb mode.  "v6"
>   ; for ARM or Thumb-2 with arm_arch6, and nov6 for ARM without
> -; arm_arch6.  This attribute is used to compute attribute "enabled",
> -; use type "any" to enable an alternative in all cases.
> -(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
> +; arm_arch6.  "v6t2" for Thumb-2 with arm_arch6.  This attribute is
> +; used to compute attribute "enabled", use type "any" to enable an
> +; alternative in all cases.
> +(define_attr "arch" "any,a,t,32,t1,t2,v6,nov6,v6t2,neon_for_64bits,avoid_neon_for_64bits,iwmmxt,iwmmxt2,armv6_or_vfpv3"
>     (const_string "any"))
>
>   (define_attr "arch_enabled" "no,yes"
> @@ -162,6 +163,10 @@
>   	      (match_test "TARGET_32BIT && !arm_arch6"))
>   	 (const_string "yes")
>
> +	 (and (eq_attr "arch" "v6t2")
> +	      (match_test "TARGET_32BIT && arm_arch6 && arm_arch_thumb2"))
> +	 (const_string "yes")
> +
>   	 (and (eq_attr "arch" "avoid_neon_for_64bits")
>   	      (match_test "TARGET_NEON")
>   	      (not (match_test "TARGET_PREFER_NEON_64BITS")))
> @@ -6288,8 +6293,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 "general_operand"      "rIk,K,n,r,mi"))]
>     "TARGET_ARM
>      && arm_arch4
>      && (register_operand (operands[0], HImode)
> @@ -6297,16 +6302,19 @@
>     "@
>      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 "arch" "*,*,v6t2,*,*")
>      (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")])]
>   )

Attachment: p5.txt
Description: Text document