| Message ID | 81462c6130048c245ec740faca7610f01ba1f1e8.1484444528.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 01/14/2017 06:55 PM, Segher Boessenkool wrote: > As shown in the PR, ifcvt will happily make invalid insns when given the > chance. This patch teaches it some manners. > > Bootstrapped and tested on powerpc64-linux. Is this okay for trunk? > > > Segher > > > 2017-01-15 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/78751 > * ifcvt.c (find_cond_trap): If we generated a non-existing insn, > give up. But isn't this a backend failing? ISTM that gen_cond_trap just calls standard expansion routines and they should either have generated a valid sequence or NULL. What in particular generated insn 64 and shouldn't it instead have indicated a failure? Jeff
On Sat, Jan 14, 2017 at 09:23:45PM -0700, Jeff Law wrote: > But isn't this a backend failing? ISTM that gen_cond_trap just calls > standard expansion routines and they should either have generated a > valid sequence or NULL. > > What in particular generated insn 64 and shouldn't it instead have > indicated a failure? ifcvt calls gen_cond_trap which then calls prepare_cmp_insn, which then calls prepare_operand on the two arms, which ends up as copy_to_mode_reg IIRC. The instructions copy_to_mode_reg comes up with are *not* valid machine insns; their RHS is ultimately given by what ifcvt called gen_cond_trap with (the comparison of the *two* RHS together is a valid instruction! -- well, a splitter; but there is no such trap insn). I have tried to handle things in expand, but it is a) ugly and b) caused problems all over the place. I wonder if anyone can still modify any of the expander code, sigh -- lots of rope there, and it's a maze. ifcvt already has a lot of that "if we made a bad insn, back out", so it fits well here. This patch also is much less risky. Segher
On 01/14/2017 10:11 PM, Segher Boessenkool wrote: > On Sat, Jan 14, 2017 at 09:23:45PM -0700, Jeff Law wrote: >> But isn't this a backend failing? ISTM that gen_cond_trap just calls >> standard expansion routines and they should either have generated a >> valid sequence or NULL. >> >> What in particular generated insn 64 and shouldn't it instead have >> indicated a failure? > > ifcvt calls gen_cond_trap which then calls prepare_cmp_insn, which then > calls prepare_operand on the two arms, which ends up as copy_to_mode_reg > IIRC. The instructions copy_to_mode_reg comes up with are *not* valid > machine insns; their RHS is ultimately given by what ifcvt called > gen_cond_trap with (the comparison of the *two* RHS together is a valid > instruction! -- well, a splitter; but there is no such trap insn). So it's the setup bits done by prepare_cmp_insn that are the problem and thus the test in gen_cond_trap isn't going to help. Ugh. AFAICT prepare_cmp_insn, through a series of calls, ends up in emit_move_insn which is just going to blindly emit the move and we've lost. Sigh. OK. Let's go with your patch. Thanks, jeff
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 68c1a1d..6d30639 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -4686,6 +4686,11 @@ find_cond_trap (basic_block test_bb, edge then_edge, edge else_edge) if (seq == NULL) return FALSE; + /* If that results in an invalid insn, back out. */ + for (rtx_insn *x = seq; x; x = NEXT_INSN (x)) + if (recog_memoized (x) < 0) + return FALSE; + /* Emit the new insns before cond_earliest. */ emit_insn_before_setloc (seq, cond_earliest, INSN_LOCATION (trap));