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.
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:
/* Check for simple loops. */
desc = get_simple_loop_desc (loop);
/* Check simpleness. */
if (!desc->simple_p || desc->assumptions)
{
if (dump_file)
fprintf (dump_file,
";; Unable to prove that the number of iterations "
"can be counted in runtime\n");
return;
}
(Yes, there's a lot of duplicated code in decide_unroll_*_iterations.)
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.
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...
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.
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.
Does this help?
Aldy
