On 02/15/2017 03:49 AM, Aldy Hernandez wrote:
On 02/13/2017 07:15 PM, Jeff Law wrote:
So it seems in your updated patch there is only one call where we ask
for LOOP_EXIT_COMPLEX, specifically the call from get_loop_location.
But I don't see how asking for LOOP_EXIT_COMPLEX from that location
would change whether or not we unroll any given loop (which is the core
of bz64081).
Am I missing something?
Ughh, only the spaghetti that is this code? ;-).
get_loop_location is only called once in the compiler, in
decide_unrolling(). This call to get_loop_location() will set the loop
description, particularly desc->simple_p where you point out.
The "desc" persists, hanging off the loop structure and its the
existence (or lack thereof) which enables/disables unrolling.
ie, we ask for complex exits via the get_loop_location calls. So loops
with complex exits will get a DESC and thus will get unrolled. Other
unpleasant things may happen as a result of having a cached DESC as well.
If we're going to go this direction, I think we need to store the exit
type in the DESC, then verify that if we ask for simple exits, then only
give back a DESC for loops with simple exits. If we ask for complex
exits, then we can give back any cached DESC.
Then every instance where we ask for a desc has to be checked for
whether or not it is safe in the presence of complex exits and ask for
the appropriate style.
Later on down in decide_unrolling(), we decide the number of iterations,
and use desc->simple_p to ignore the loop if it is not simple.
decide_unroll_constant_iterations (loop, flags);
if (loop->lpt_decision.decision == LPT_NONE)
decide_unroll_runtime_iterations (loop, flags);
if (loop->lpt_decision.decision == LPT_NONE)
decide_unroll_stupid (loop, flags);
Any one of these functions will bail if the loop description was not
simple_p:
I don't think so. If that were the case, then we'd never unroll the
loop in the testcase. In fact, decide_unroll_constant_iterations
unrolls the loop with the complex exit.
Now a problem I see here is that decide_unroll_*_iterations all call
get_simple_loop_desc() which is basically LOOP_EXIT_SIMPLE, but since
the value is already cached we return the previous call that was
LOOP_EXIT_COMPLEX. So the code works because we will already have a
cached value.
Right. And ISTM it's the existence of the DESC structure that's the key
here. Right?
I think to make it clearer we could:
1. Add an assert in get_loop_desc to make sure that if we're returning a
cached loop description, that the LOOP_EXIT_TYPEs match. Just in case...
Probably not a bad idea. But you don't save the loop exit type AFAICT.
In fact, if you did, you'd quickly see that the calls via decide_* would
likely trigger your assert.
2. Change all the decide_unroll_*_iterations variants to specifically
ask for a LOOP_EXIT_TYPE, not just the simple variant. And have this
set to LOOP_EXIT_COMPLEX from decide_unrolling. Right now, this is all
working because we have only one call to get_loop_location, but I assume
that could change.
But the simple variants all ask for LOOP_EXIT_SIMPLE, so I don't see
this as improving things significantly.
3. And finally, what the heck is get_loop_location doing in cfgloop,
when it's only used once within loop-unroll.c? I say we move it to
loop-unroll.c and mark it static.
Seems like a reasonable cleanup.
Does this help?
It does. But it really shows that the introduction of simple vs complex
exits is messed up. All we've done is add another layer of complexity
that happens to work IMHO, but it seems to do so more by accident than
design.
Jeff
