This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] make canonicalize_condition keep its promise


On 11/21/2017 10:45 AM, Aaron Sawdey wrote:
> On Tue, 2017-11-21 at 10:06 -0700, Jeff Law wrote:
>> On 11/20/2017 06:41 AM, Aaron Sawdey wrote:
>>> On Sun, 2017-11-19 at 16:44 -0700, Jeff Law wrote:
>>>> 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?
>>>
>>> The rtl to use this instruction is generated by new code I'm
>>> working on
>>> to do a builtin expansion of memcmp using a loop. I call
>>> gen_bdnztf_di() to generate rtl for the insn. It would be nice to
>>> be
>>> able to generate this instruction from doloop conversion but that
>>> is
>>> beyond the scope of what I'm working on presently.
>>
>> Understood.
>>
>> So what I think (and I'm hoping you can confirm one way or the other)
>> is
>> that by generating this instruction you're turing a loop which
>> previously was considered a simple loop by the IV code and turning it
>> into something the IV bits no longer think is a simple loop.
>>
>> I think that's problematical as when the loop is thought to be a
>> simple
>> loop, it has to have a small number of forms for its loop back/exit
>> loop
>> tests and whether or not a loop is a simple loop is cached in the
>> loop
>> structure.
>>
>> I think we need to dig into that first.  If my suspicion is correct
>> then
>> this patch is really just papering over that deeper problem.  So I
>> think
>> you need to dig a big deeper into why you're getting into the code in
>> question (canonicalize_condition) and whether or not the call chain
>> makes any sense given the changes you've made to the loop.
>>
> 
> Jeff,
>   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.

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

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.


Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]