Hi Jeff,
On Tue, Dec 19, 2017 at 04:40:23PM -0700, Jeff Law wrote:
> On 12/15/2017 02:16 AM, Segher Boessenkool wrote:
> >> The only way to get into check_simple_exit is via find_simple_exit which
> >> is only called from get_simple_loop_desc.
> >>
> >> And if you're calling get_simple_loop_desc, then there is some kind of
> >> loop structure in place AFAICT that contains this insn which is rather
> >> surprising.
> >
> > Why? It *is* a loop!
> Right. But how was the loop ever considered simple given that kind of
> test? At one time I managed to convince myself that to trigger the
> calls Aaron is having trouble with we must have already analyzed the
> loop as "simple" and I'm just having a hard time seeing how that could
> be the case given the from of that insn.
loop-iv.c:check_simple_exit has these two relevant things:
1)
/* It must end in a simple conditional jump. */
if (!any_condjump_p (BB_END (exit_bb)))
return;
any_condjump_p just sees if it is assigning a suitable if_then_else to pc.
It is. The comment on any_condjump_p says:
/* Return true when insn is a conditional jump. This function works for
instructions containing PC sets in PARALLELs. The instruction may have
various other effects so before removing the jump you must verify
onlyjump_p.
Note that unlike condjump_p it returns false for unconditional jumps. */
bdnzt and friends are a parallel of a conditional jump, with as condition
checking whether ctr (the "counter register") will reach 0 (or not), and
whether some other condition is true (or not); and as the other arm of the
parallel it decrements ctr.
2)
/* Test whether the condition is suitable. */
if (!(condition = get_condition (BB_END (ein->src), &at, false, false)))
return;
... and that gets into trouble already: it gets the condition just fine,
but then it calls canonicalize_condition on that, which cannot handle it.
Which is what Aaron's patch fixes.
> >> However, I'm still concerned about how we got to a point where this is
> >> happening. So while we can fix canonicalize_condition to reject this
> >> form (and you can argue we should and I'd generally agree with you), it
> >> could well be papering over a problem earlier.
> >
> > canonicalize_condition does not do what its documentation says it does.
> > Fixing that is not papering over a problem. Of course there could be a
> > problem elsewhere, sure. But *this* problem is blocking Aaron's other
> > patches right now (which are approved and ready to go in).
> Given that I don't think we should be getting into
> canonicalize_condition at all, "fixing it" *is* papering over the problem.
>
> So I think the way forward is to show that the way we're getting into
> canonicalize_condition is reasonable/valid. Once that happens we can go
> forward with Aaron's patch.
Let me just paste all of get_condition then (it is very short):
rtx
get_condition (rtx_insn *jump, rtx_insn **earliest, int allow_cc_mode,
int valid_at_insn_p)
{
rtx cond;
int reverse;
rtx set;
/* If this is not a standard conditional jump, we can't parse it. */
if (!JUMP_P (jump)
|| ! any_condjump_p (jump))
return 0;
set = pc_set (jump);
cond = XEXP (SET_SRC (set), 0);
/* If this branches to JUMP_LABEL when the condition is false, reverse
the condition. */
reverse
= GET_CODE (XEXP (SET_SRC (set), 2)) == LABEL_REF
&& label_ref_label (XEXP (SET_SRC (set), 2)) == JUMP_LABEL (jump);
return canonicalize_condition (jump, cond, reverse, earliest, NULL_RTX,
allow_cc_mode, valid_at_insn_p);
}
All of this is totally reasonable and expected; "cond" is an AND of two
things, which canonicalize_condition cannot handle, so it should return
NULL.
Segher
