Created attachment 35200 [details] test case When compile attached case with command: arm-none-eabi-gcc -march=armv6-m -mthumb -Os x.c -S The gcc generates bad code as below: main: push {r0, r1, r2, r4, r5, r6, r7, lr} ldr r4, .L5 movs r5, #0 ldr r0, [r4, #8] add r1, sp, #4 rsbs r2, r0, #0 adcs r2, r2, r0 subs r3, r2, r3 //r3 is used but not initialized The instruction to initialize r3 is removed due to IRA failed to recognize the clobber in reload pass. Before the reload pass, we have three instructions like below: (insn 12 11 14 2 (parallel [ (set (reg:SI 128) (eq:SI (reg:SI 110 [ D.4914 ]) (const_int 0 [0]))) (clobber (reg:SI 129)) ]) x.c:23 760 {*cstoresi_eq0_thumb1_insn} (expr_list:REG_UNUSED (reg:SI 129) (nil))) (insn 20 19 22 2 (set (reg:SI 135) (plus:SI (plus:SI (reg:SI 136) (reg:SI 137)) (geu:SI (reg:SI 131 [ D.4914 ]) (reg:SI 134 [ c ])))) x.c:23 764 {thumb1_addsi3_addgeu} (expr_list:REG_DEAD (reg:SI 137) (expr_list:REG_DEAD (reg:SI 136) (expr_list:REG_DEAD (reg:SI 134 [ c ]) (expr_list:REG_DEAD (reg:SI 131 [ D.4914 ]) (nil)))))) (insn 22 20 23 2 (set (reg:SI 138) (minus:SI (reg:SI 128) (reg:SI 135))) x.c:23 720 {thumb1_subsi3_insn} (expr_list:REG_DEAD (reg:SI 135) (expr_list:REG_DEAD (reg:SI 128) (nil)))) After the reload pass, those instructions are wrongly turned into: (insn 20 53 58 2 (set (reg:SI 3 r3 [135]) (plus:SI (plus:SI (reg:SI 3 r3 [137]) (reg:SI 2 r2 [136])) (geu:SI (reg:SI 7 r7 [orig:131 D.4914 ] [131]) (reg:SI 6 r6 [153])))) x.c:23 764 {thumb1_addsi3_addgeu} (nil)) (insn 58 20 55 2 (parallel [ (set (reg:SI 2 r2 [128]) (eq:SI (reg:SI 0 r0 [orig:110 D.4914 ] [110]) (const_int 0 [0]))) (clobber (reg:SI 3 r3 [129])) ]) x.c:23 760 {*cstoresi_eq0_thumb1_insn} (nil)) (note 55 58 22 2 NOTE_INSN_DELETED) (insn 22 55 23 2 (set (reg:SI 3 r3 [138]) (minus:SI (reg:SI 2 r2 [128]) (reg:SI 3 r3 [135]))) x.c:23 720 {thumb1_subsi3_insn} (nil)) However the later pass can recognize the clobber in insn 58 and will remove the insn 20 because the r3 defined in insn 20 is clobbered by insn 58.
On Wed, Apr 1, 2015 at 5:44 PM, terry.guo at arm dot com <gcc-bugzilla@gcc.gnu.org> wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65648 > > Bug ID: 65648 > Summary: [5 Regression] Bad code due to IRA fails to recognize > the clobber in parallel > Product: gcc > Version: 5.0 > Status: UNCONFIRMED > Severity: normal > Priority: P3 > Component: target > Assignee: unassigned at gcc dot gnu.org > Reporter: terry.guo at arm dot com > > Created attachment 35200 [details] > --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35200&action=edit > test case > > When compile attached case with command: > arm-none-eabi-gcc -march=armv6-m -mthumb -Os x.c -S > > The gcc generates bad code as below: > main: > push {r0, r1, r2, r4, r5, r6, r7, lr} > ldr r4, .L5 > movs r5, #0 > ldr r0, [r4, #8] > add r1, sp, #4 > rsbs r2, r0, #0 > adcs r2, r2, r0 > subs r3, r2, r3 //r3 is used but not initialized > > The instruction to initialize r3 is removed due to IRA failed to recognize the > clobber in reload pass. Before the reload pass, we have three instructions like > below: > (insn 12 11 14 2 (parallel [ > (set (reg:SI 128) > (eq:SI (reg:SI 110 [ D.4914 ]) > (const_int 0 [0]))) > (clobber (reg:SI 129)) > ]) x.c:23 760 {*cstoresi_eq0_thumb1_insn} > (expr_list:REG_UNUSED (reg:SI 129) > (nil))) > (insn 20 19 22 2 (set (reg:SI 135) > (plus:SI (plus:SI (reg:SI 136) > (reg:SI 137)) > (geu:SI (reg:SI 131 [ D.4914 ]) > (reg:SI 134 [ c ])))) x.c:23 764 {thumb1_addsi3_addgeu} > (expr_list:REG_DEAD (reg:SI 137) > (expr_list:REG_DEAD (reg:SI 136) > (expr_list:REG_DEAD (reg:SI 134 [ c ]) > (expr_list:REG_DEAD (reg:SI 131 [ D.4914 ]) > (nil)))))) > (insn 22 20 23 2 (set (reg:SI 138) > (minus:SI (reg:SI 128) > (reg:SI 135))) x.c:23 720 {thumb1_subsi3_insn} > (expr_list:REG_DEAD (reg:SI 135) > (expr_list:REG_DEAD (reg:SI 128) > (nil)))) > > After the reload pass, those instructions are wrongly turned into: > (insn 20 53 58 2 (set (reg:SI 3 r3 [135]) > (plus:SI (plus:SI (reg:SI 3 r3 [137]) > (reg:SI 2 r2 [136])) > (geu:SI (reg:SI 7 r7 [orig:131 D.4914 ] [131]) > (reg:SI 6 r6 [153])))) x.c:23 764 {thumb1_addsi3_addgeu} > (nil)) > (insn 58 20 55 2 (parallel [ > (set (reg:SI 2 r2 [128]) > (eq:SI (reg:SI 0 r0 [orig:110 D.4914 ] [110]) > (const_int 0 [0]))) > (clobber (reg:SI 3 r3 [129])) > ]) x.c:23 760 {*cstoresi_eq0_thumb1_insn} > (nil)) > (note 55 58 22 2 NOTE_INSN_DELETED) > (insn 22 55 23 2 (set (reg:SI 3 r3 [138]) > (minus:SI (reg:SI 2 r2 [128]) > (reg:SI 3 r3 [135]))) x.c:23 720 {thumb1_subsi3_insn} > (nil)) > > However the later pass can recognize the clobber in insn 58 and will remove the > insn 20 because the r3 defined in insn 20 is clobbered by insn 58. This is a bug in the arm's thumb1.md file, it uses match_operand when it should really be using match_scratch. Looks like most of the arm back-end has this bug in it. See https://gcc.gnu.org/onlinedocs/gccint/Regs-and-Memory.html#index-scratch-2870 and the corresponding match_scratch documentation. That is this: 1515 (define_insn "*cstoresi_eq0_thumb1_insn" 1516 [(set (match_operand:SI 0 "s_register_operand" "=&l,l") 1517 (eq:SI (match_operand:SI 1 "s_register_operand" "l,0") 1518 (const_int 0))) 1519 (clobber (match_operand:SI 2 "s_register_operand" "=X,l"))] 1520 "TARGET_THUMB1" 1521 "@ 1522 rsbs\\t%0, %1, #0\;adcs\\t%0, %0, %1 1523 rsbs\\t%2, %1, #0\;adcs\\t%0, %1, %2" 1524 [(set_attr "length" "4") 1525 (set_attr "type" "multiple")] 1526 ) Really should be using (clobber (match_scratch:... )) So it generates a scratch RTL right away and then the register allocater knows that it is only used in that instruction. Thanks, Andrew
Well, I think the real issue here is LRA deciding to move insn 12 regardless of whether or not the MD file gets a scratch register the recommend way or not. Note, the insns below are not consecutive in the .ira dump, had me rather confused for a few minutes.
Author: hubicka Date: Fri Apr 3 20:25:01 2015 New Revision: 221861 URL: https://gcc.gnu.org/viewcvs?rev=221861&root=gcc&view=rev Log: PR ipa/65648 * ipa-inline-transform.c (inline_call): Skip sanity check to work around the ICE Modified: trunk/gcc/ChangeLog trunk/gcc/ipa-inline-transform.c
(In reply to Jeffrey A. Law from comment #2) > Well, I think the real issue here is LRA deciding to move insn 12 regardless > of whether or not the MD file gets a scratch register the recommend way or > not. > > Note, the insns below are not consecutive in the .ira dump, had me rather > confused for a few minutes. The insn looks moved as the bug is in new rematerialization sub-pass of LRA. I'll try to fix this until Wednesday.
Author: vmakarov Date: Tue Apr 7 15:01:07 2015 New Revision: 221901 URL: https://gcc.gnu.org/viewcvs?rev=221901&root=gcc&view=rev Log: 2015-04-07 Vladimir Makarov <vmakarov@redhat.com> PR target/65678 * lra-remat.c (do_remat): Process input and non-input insn registers separately. Modified: trunk/gcc/ChangeLog trunk/gcc/lra-remat.c
Thus fixed, still would be nice to have a testcase in the testsuite. Terry or other ARM folks, can you please help with that? See http://gcc.gnu.org/ml/gcc-patches/2015-04/msg00262.html for more details.
(In reply to Jakub Jelinek from comment #6) > Thus fixed, still would be nice to have a testcase in the testsuite. Terry > or other ARM folks, can you please help with that? > See http://gcc.gnu.org/ml/gcc-patches/2015-04/msg00262.html for more details. It seems to me that Yvan is working on this, please refer to https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00272.html.
Author: yroux Date: Fri Apr 10 19:03:27 2015 New Revision: 221981 URL: https://gcc.gnu.org/viewcvs?rev=221981&root=gcc&view=rev Log: Add missing testcase. 2015-04-19 Yvan Roux <yvan.roux@linaro.org> PR target/65648 * gcc.target/arm/pr65647-2.c: New. Added: trunk/gcc/testsuite/gcc.target/arm/pr65647-2.c Modified: trunk/gcc/testsuite/ChangeLog
Author: yroux Date: Tue Apr 14 12:12:29 2015 New Revision: 222083 URL: https://gcc.gnu.org/viewcvs?rev=222083&root=gcc&view=rev Log: Add missing testcase. 2015-04-14 Yvan Roux <yvan.roux@linaro.org> PR target/65648 * gcc.c-torture/execute/pr65648.c: New test. Added: trunk/gcc/testsuite/gcc.c-torture/execute/pr65648.c Modified: trunk/gcc/testsuite/ChangeLog