| Message ID | 20140929053627.GZ17454@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/28/14 23:36, Jakub Jelinek wrote: > Hi! > > On the following testcase, dead_or_predicable decides to call > redirect_jump_1 on asm goto which has more than one label, but the > bb still has just two successors (one of the labels points to > code label at the start of the fallthru bb) and redirect_jump_1 > ICEs on that. > > Usually dead_or_predicable fails if !any_condjump_p, but there > is a shortcut (goto no_body) that bypasses that. > > I think it doesn't really make sense to allow anything but normal > conditional jumps here, so the first patch just gives up in that > case. Have done instrumented bootstrap on > {i?86,x86_64,aarch64,armv7hl,ppc64,ppc64le,s390,s390x}-linux > with this and the added goto cancel didn't trigger in any of the > bootstraps, and triggered only on 1-3 testcases in the testsuite > which all had asm goto in them (one of them this newly added > testcase). > > Alternately, the second patch turns an assert in redirect_jump_1 > into return 0, so it will fail (that also fixes the testcase). > With that patch alone, I'm worried about dead_or_predicable calling > invert_jump_1 on asm goto, which I can't understand how it would work > (there is no way how the "condition" can be inverted). So, if > the second patch is preferable, I think dead_or_predicable should > still goto cancel if (reversep && !any_condjump_p (jump)). Or > invert_jump_1 should fail early if !any_condjump_p. Or both > of the patches could be applied together as is (of course, testcase > just from one of those). > > 2014-09-29 Jakub Jelinek <jakub@redhat.com> > > PR inline-asm/63282 > * ifcvt.c (dead_or_predicable): Don't call redirect_jump_1 > or invert_jump_1 if jump isn't any_condjump_p. > > * gcc.c-torture/compile/pr63282.c: New test. I think restricting to normal jumps is fine. Approved. jeff
--- gcc/ifcvt.c.jj 2014-09-12 09:29:21.000000000 +0200 +++ gcc/ifcvt.c 2014-09-26 20:50:08.610965141 +0200 @@ -4357,6 +4357,9 @@ dead_or_predicable (basic_block test_bb, old_dest = JUMP_LABEL (jump); if (other_bb != new_dest) { + if (!any_condjump_p (jump)) + goto cancel; + if (JUMP_P (BB_END (dest_edge->src))) new_dest_label = JUMP_LABEL (BB_END (dest_edge->src)); else if (new_dest == EXIT_BLOCK_PTR_FOR_FN (cfun)) --- gcc/testsuite/gcc.c-torture/compile/pr63282.c.jj 2014-09-26 20:51:40.999482179 +0200 +++ gcc/testsuite/gcc.c-torture/compile/pr63282.c 2014-09-26 20:51:43.882467325 +0200 @@ -0,0 +1,13 @@ +/* PR inline-asm/63282 */ + +void bar (void); + +void +foo (void) +{ + asm volatile goto ("" : : : : a, b); +a: + bar (); +b: + return; +}
Hi! On the following testcase, dead_or_predicable decides to call redirect_jump_1 on asm goto which has more than one label, but the bb still has just two successors (one of the labels points to code label at the start of the fallthru bb) and redirect_jump_1 ICEs on that. Usually dead_or_predicable fails if !any_condjump_p, but there is a shortcut (goto no_body) that bypasses that. I think it doesn't really make sense to allow anything but normal conditional jumps here, so the first patch just gives up in that case. Have done instrumented bootstrap on {i?86,x86_64,aarch64,armv7hl,ppc64,ppc64le,s390,s390x}-linux with this and the added goto cancel didn't trigger in any of the bootstraps, and triggered only on 1-3 testcases in the testsuite which all had asm goto in them (one of them this newly added testcase). Alternately, the second patch turns an assert in redirect_jump_1 into return 0, so it will fail (that also fixes the testcase). With that patch alone, I'm worried about dead_or_predicable calling invert_jump_1 on asm goto, which I can't understand how it would work (there is no way how the "condition" can be inverted). So, if the second patch is preferable, I think dead_or_predicable should still goto cancel if (reversep && !any_condjump_p (jump)). Or invert_jump_1 should fail early if !any_condjump_p. Or both of the patches could be applied together as is (of course, testcase just from one of those). 2014-09-29 Jakub Jelinek <jakub@redhat.com> PR inline-asm/63282 * ifcvt.c (dead_or_predicable): Don't call redirect_jump_1 or invert_jump_1 if jump isn't any_condjump_p. * gcc.c-torture/compile/pr63282.c: New test. Jakub 2014-09-26 Jakub Jelinek <jakub@redhat.com> PR inline-asm/63282 * jump.c (redirect_jump_1): If ASM_OPERANDS_LABEL_LENGTH (asmop) is not 1, return 0. * gcc.c-torture/compile/pr63282.c: New test. --- gcc/jump.c.jj 2014-09-16 10:00:36.000000000 +0200 +++ gcc/jump.c 2014-09-26 20:54:46.174519857 +0200 @@ -1510,9 +1510,8 @@ redirect_jump_1 (rtx jump, rtx nlabel) asmop = extract_asm_operands (PATTERN (jump)); if (asmop) { - if (nlabel == NULL) + if (nlabel == NULL || ASM_OPERANDS_LABEL_LENGTH (asmop) != 1) return 0; - gcc_assert (ASM_OPERANDS_LABEL_LENGTH (asmop) == 1); loc = &ASM_OPERANDS_LABEL (asmop, 0); } else if (GET_CODE (PATTERN (jump)) == PARALLEL) --- gcc/testsuite/gcc.c-torture/compile/pr63282.c.jj 2014-09-26 20:51:40.999482179 +0200 +++ gcc/testsuite/gcc.c-torture/compile/pr63282.c 2014-09-26 20:51:43.882467325 +0200 @@ -0,0 +1,13 @@ +/* PR inline-asm/63282 */ + +void bar (void); + +void +foo (void) +{ + asm volatile goto ("" : : : : a, b); +a: + bar (); +b: + return; +}