Re: [PATCH] make canonicalize_condition keep its promise

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

On Thu, Dec 14, 2017 at 01:43:35PM -0700, Jeff Law wrote:
> On 11/21/2017 10:45 AM, Aaron Sawdey wrote:
> >   There is no existing loop structure. This starts with a memcmp() call
> > and then goes down through the builtin expansion mechanism, which is
> > ultimately expanding the pattern cmpmemsi which is where my code is
> > generating a loop that finishes with bdnzt. The code that's ultimately
> > generated looks like this:
> Understood.  But what I still struggle with is how you're getting into
> check_simple_exit to begin with and whether or not that should be happening.
> 
> 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!

Or are you wondering why loop-iv.c is involved?  get_simple_loop_desc
in there is also called from much later in RTL passes.

> > I really think the ultimate problem here is that both
> > canonicalize_condition and get_condition promise in their documenting
> > comments that they will return something that has a cond at the root of
> > the rtx, or 0 if they don't understand what they're given. In this case
> > they do not understand the rtx of bdnzt and are returning rtx rooted
> > with an and, not a cond. This may seem like papering over the problem,
> > but I think it is legitimate for these functions to return 0 when the
> > branch insn in question does not have a simple cond at the heart of it.
> > And bootstrap/regtest did pass with my patch on ppc64le and x86_64.
> > Ultimately, yes something better ought to be done here.
> 
> 
> 
> Your pattern has the form:
> 
>   [(set (pc)
> 	(if_then_else
> 	  (and
> 	     (ne (match_operand:P 1 "register_operand" "c,*b,*b,*b")
> 		 (const_int 1))
> 	     (match_operator 3 "branch_comparison_operator"
> 		      [(match_operand 4 "cc_reg_operand" "y,y,y,y")
> 		       (const_int 0)]))
> 		      (label_ref (match_operand 0))
> 		      (pc)))
>    (set (match_operand:P 2 "nonimmediate_operand" "=1,*r,m,*d*wi*c*l")
> 	(plus:P (match_dup 1)
> 		(const_int -1)))
>    (clobber (match_scratch:P 5 "=X,X,&r,r"))
>    (clobber (match_scratch:CC 6 "=X,&y,&y,&y"))
>    (clobber (match_scratch:CCEQ 7 "=X,&y,&y,&y"))]
> 
> 
> 
> That's a form that get_condition knows how to parse.  It's going to pull
> out the condition which looks like this:
> 
> 
> 	  (and
> 	     (ne (match_operand:P 1 "register_operand" "c,*b,*b,*b")
> 		 (const_int 1))
> 	     (match_operator 3 "branch_comparison_operator"
> 		      [(match_operand 4 "cc_reg_operand" "y,y,y,y")
> 		       (const_int 0)]))
> 
> ANd pass that down to canonicalize_condition.  That doesn't look like
> something canonicalize_condition should handle and thus it ought to be
> returning NULL_RTX.

Yes exactly.

> 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).


Segher