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