Re: [PATCH] make canonicalize_condition keep its promise

[Jeff Law <law at redhat dot com>]

On 11/29/2017 06:24 PM, Aaron Sawdey wrote:
> On Tue, 2017-11-21 at 11:45 -0600, 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:
>>
>>         srdi 9,10,4
>>         li 6,0
>>         mtctr 9
>>         li 4,8
>> .L9:
>>         ldbrx 8,11,6
>>         ldbrx 9,7,6
>>         ldbrx 5,11,4
>>         ldbrx 3,7,4
>>         addi 6,6,16
>>         addi 4,4,16
>>         subfc. 9,9,8
>>         bne 0,.L4
>>         subfc. 9,3,5
>>         bdnzt 2,.L9
>>
>> So it is a loop with a branch out, and then the branch decrement nz
>> true back to the top. The iv's here (regs 4 and 6) were generated by
>> my
>> expansion code. 
>>
>> 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.
>>
> 
> Jeff,
>   Just wondering if you would have time to take another look at this
> now that Thanksgiving is past. I'm hoping to get this resolved so I can
> get this new memcmp expansion code into gcc8.
Definitely in the queue to revisit -- last 3 days have had limited time
to deal with GCC stuff

jeff