Re: [PATCH] Fix for PR 61561

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

On 19/06/14 16:12, Kyrill Tkachov wrote:
> On 19/06/14 16:05, Marat Zakirov wrote:
> > Hi all,
> >
> > Here's a patch for PR 61561
> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61561).
> >
> > It fixes ICE.

Thanks for your contribution.

However, this is *really* not the way to submit a patch and is the sort 
of patch that will make me avoid reviewing it instantly.

But given this is your first time ...... :)

Can you please explain why your fix is required ? One can understand 
what you are attempting to achieve which is adding constraints and 
alternatives for moves between sp and general purpose registers for 
HImode and QImode operands, but how did we get here in the first place ? 
My next reaction was which pass is doing this and why ?

Ah, then I go and read your bug report and your submission there in the 
description. Now I understand.

Please read - https://gcc.gnu.org/contribute.html#patches


> > Reg. tested on arm15.

What's an ARM15 ? I have never seen it or even heard about it. 
Presumably you mean a Cortex-A15 and that is inferred from your comments 
in the bugzilla report.

What configuration did you test, languages ?


Now on to the patch itself.


> gcc/ChangeLog:
>
> 2014-06-19  Marat Zakirov  <m.zakirov@samsung.com>
>
> 	* config/arm/arm.md: New templates see pr61561.

See Changelog formats and comments about using PR numbers in Changelogs.

<DATE>  <NAME>  <email>

	PR target/61561
	* config/arm/arm.md (*movhi_insn_arch4): Handle stack pointer
	  (*arm_movqi_insn): Likewise.
> gcc/testsuite/ChangeLog:
>
> 2014-06-19  Marat Zakirov  <m.zakirov@samsung.com>
>
> 	* c-c++-common/pr61561.c: New test for pr61561.

PR target/61561
* <file_name>: New test.

> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 42c12c8..7ed8abc 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -6290,27 +6290,31 @@
>
>  ;; 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"      "rI,K,r,mi"))]
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,m,r,r")
> +	(match_operand:HI 1 "general_operand"      "rI,K,r,mi,k"))]
>    "TARGET_ARM
>     && arm_arch4
>     && (register_operand (operands[0], HImode)
>         || register_operand (operands[1], HImode))"
> -  "@
> +  "@
>     mov%?\\t%0, %1\\t%@ movhi
>     mvn%?\\t%0, #%B1\\t%@ movhi
>     str%(h%)\\t%1, %0\\t%@ movhi
> -   ldr%(h%)\\t%0, %1\\t%@ movhi"
> +   ldr%(h%)\\t%0, %1\\t%@ movhi
> +   uxth%?\\t%0, %1\\t%@ movhi"

Instead replace the first source constraint with rIk rather than adding 
another alternative. You don't need a widening operation here. There is 
no need to do a zero extend here. Any widening or narrowing should be 
dealt with where required, and the store would remain a store byte and 
therefore this can be a move like the other moves between registers.

This pattern matches for arm_arch4 where uxth is not a valid 
instruction. Further more on earlier architectures this will cause an 
undefined instruction trap.



>    [(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_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 "store1")
> -                          (const_string "load1")])]
> +                          (const_string "load1")
> +                          (if_then_else (match_operand 1 "const_int_operand" "")
> +                                        (const_string "mov_imm" )
> +                                        (const_string "mov_reg"))])]


This should not be needed now. This is just a move_reg.

>  )
>
>  (define_insn "*movhi_bytes"
> @@ -6429,8 +6433,8 @@
>  )
>
>  (define_insn "*arm_movqi_insn"
> -  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,r,l,r,l,Uu,r,m")
> -	(match_operand:QI 1 "general_operand" "r,r,I,Py,K,Uu,l,m,r"))]
> +  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,r,l,r,l,Uu,r,m,r,r")
> +	(match_operand:QI 1 "general_operand" "r,r,I,Py,K,Uu,l,m,r,k,k"))]

Why do you need 2 alternatives which appear to do the same thing ? 
Instead just do a move.



>    "TARGET_32BIT
>     && (   register_operand (operands[0], QImode)
>         || register_operand (operands[1], QImode))"
> @@ -6443,12 +6447,14 @@
>     ldr%(b%)\\t%0, %1
>     str%(b%)\\t%1, %0
>     ldr%(b%)\\t%0, %1
> -   str%(b%)\\t%1, %0"
> -  [(set_attr "type" "mov_reg,mov_reg,mov_imm,mov_imm,mvn_imm,load1,store1,load1,store1")
> +   str%(b%)\\t%1, %0
> +   uxtb%?\\t%0, %1
> +   uxtb%?\\t%0, %1"

> +  [(set_attr "type" "mov_reg,mov_reg,mov_imm,mov_imm,mvn_imm,load1,store1,load1,store1,mov_reg,mov_reg")
>     (set_attr "predicable" "yes")
> -   (set_attr "predicable_short_it" "yes,yes,yes,no,no,no,no,no,no")
> -   (set_attr "arch" "t2,any,any,t2,any,t2,t2,any,any")
> -   (set_attr "length" "2,4,4,2,4,2,2,4,4")]
> +   (set_attr "predicable_short_it" "yes,yes,yes,no,no,no,no,no,no,no,no")
> +   (set_attr "arch" "t2,any,any,t2,any,t2,t2,any,any,any,t2")
> +   (set_attr "length" "2,4,4,2,4,2,2,4,4,4,2")]
>  )
>
>  ;; HFmode moves
> diff --git a/gcc/testsuite/c-c++-common/pr61561.c b/gcc/testsuite/c-c++-common/pr61561.c
> new file mode 100644
> index 0000000..0f4b716
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/pr61561.c
> @@ -0,0 +1,15 @@
> +/* PR c/61561 */
> +/* { dg-do assemble } */
> +/* { dg-options " -w" } */

Typically we put this into gcc.dg/ or if this is a test specific to ARM 
put it in gcc.target/arm with a dg-options of -O1 / -O2. In this case 
putting this in gcc.dg is probably ok.

Alternatively put this into gcc.c-torture/execute if you want this 
tortured across multiple optimization levels ?

> +
> +int dummy(int a);
> +
> +char a;
> +short b;
> +
> +void mmm (void)
> +{
> +  char dyn[ dummy(3) ];
> +  a = (char)&dyn[0];
> +  b = (short)&dyn[0];
> +}

Please submit another patch with all these changes again.

regards
Ramana

> CC'ing the arm maintainers...
>
> Kyrill
>
> > --Marat