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! Or are you wondering why loop-iv.c is involved? get_simple_loop_desc in there is also called from much later in RTL passes. > > 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. Yes exactly. > 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). Segher
