Re: [PATCH] make canonicalize_condition keep its promise

[Jeff Law <law at redhat dot com>]

On 12/15/2017 02:16 AM, Segher Boessenkool wrote:
> 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!
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.

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

jeff