Re: [PATCH] make canonicalize_condition keep its promise

[Jeff Law <law at redhat dot com>]

On 11/15/2017 08:40 AM, Aaron Sawdey wrote:
> So, the story of this very small patch starts with me adding patterns
> for ppc instructions bdz[tf] and bdnz[tf] such as this:
> 
>   [(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"))]
> 
> However when this gets to the loop_doloop pass, we get an assert fail
> in iv_number_of_iterations():
> 
>   gcc_assert (COMPARISON_P (condition));
> 
> This is happening because this branch insn tests two things ANDed
> together so the and is at the top of the expression, not a comparison.
Is this something you've created for an existing loop?  Presumably an
existing loop that previously was a simple loop?

> 
> This condition is extracted from the insn by get_condition() which is
> pretty straightforward, and which calls canonicalize_condition() before
> returning it. Now, one could put a test for a jump condition that is
> not a conditional test in here but the comment for
> canonicalize_condition() says:
> 
>    (1) The code will always be a comparison operation (EQ, NE, GT, etc.).
> 
> So, this patch adds a test at the end that just returns 0 if the return
> rtx is not a comparison. As it happens, doloop conversion is not needed
> here because I'm already generating rtl for a branch-decrement counter
> based loop.
I'm not at all familiar with this code, but I would probably guess that
COND is supposed to appear within INSN.  Canonicalize comparison is
supposed to modify the COND expression that appears within INSN.  THe
overall structure of INSN doesn't much matter as long as COND refers to
a comparison code with two operands.

My gut tells me the problem is upstream or downstream in the call chain
or that you've taken a loop that was considered a simple loop and
modified the exit test in a way that other code is not prepared to handle.
jeff