Re: [PATCH] make canonicalize_condition keep its promise

[Segher Boessenkool <segher at kernel dot crashing dot org>]

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