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
