| Message ID | 3e964005-5e24-0f15-0c88-b833e086debf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix unrolling check. | expand |
> I was fiddling around with the loop unrolling pass and noticed a check > in decide_unroll_* functions (in the patch). The comment on top of this > check says > "/* If we were not asked to unroll this loop, just return back silently. > */" > However the check returns when loop->unroll == 0 rather than 1. > > The check was added in r255106 where the ChangeLog suggests that the > actual intention was probably to check the value 1 and not 0. No, this is intended, 0 is the default value of the field, not 1. And note that decide_unroll_constant_iterations, decide_unroll_runtime_iterations and decide_unroll_stupid *cannot* be called with loop->unroll == 1 because of this check in decide_unrolling: if (loop->unroll == 1) { if (dump_file) fprintf (dump_file, ";; Not unrolling loop, user didn't want it unrolled\n"); continue; } > Tested on aarch64-none-elf with one new regression: > FAIL: gcc.dg/pr40209.c (test for excess errors) > This fails because the changes cause the loop to unroll 3 times using > unroll_stupid and that shows up as excess error due -fopt-info. This > option was added in r202077 but I am not sure why this particular test > was chosen for it. That's a regression, there should be no unrolling.
Hi Eric On 08/11/2019 19:16, Eric Botcazou wrote: >> I was fiddling around with the loop unrolling pass and noticed a check >> in decide_unroll_* functions (in the patch). The comment on top of this >> check says >> "/* If we were not asked to unroll this loop, just return back silently. >> */" >> However the check returns when loop->unroll == 0 rather than 1. >> >> The check was added in r255106 where the ChangeLog suggests that the >> actual intention was probably to check the value 1 and not 0. > > No, this is intended, 0 is the default value of the field, not 1. And note > that decide_unroll_constant_iterations, decide_unroll_runtime_iterations and > decide_unroll_stupid *cannot* be called with loop->unroll == 1 because of this > check in decide_unrolling: Thanks for the explanation. However, I do not understand why are we returning with the default value. The comment for "unroll" is a bit ambiguous for value 0. /* The number of times to unroll the loop. 0 means no information given, just do what we always do. A value of 1 means do not unroll the loop. A value of USHRT_MAX means unroll with no specific unrolling factor. Other values means unroll with the given unrolling factor. */ unsigned short unroll; What "do we always do"? Thanks Sudi > > if (loop->unroll == 1) > { > if (dump_file) > fprintf (dump_file, > ";; Not unrolling loop, user didn't want it unrolled\n"); > continue; > } > >> Tested on aarch64-none-elf with one new regression: >> FAIL: gcc.dg/pr40209.c (test for excess errors) >> This fails because the changes cause the loop to unroll 3 times using >> unroll_stupid and that shows up as excess error due -fopt-info. This >> option was added in r202077 but I am not sure why this particular test >> was chosen for it. > > That's a regression, there should be no unrolling. >
> Thanks for the explanation. However, I do not understand why are we > returning with the default value. The regression you reported should be clear enough though: if we don't do that, we will unroll in cases where we would not have before. Try with a compiler that predates the pragma and compare, there should be no changes. > What "do we always do"? What we do in the absence of specific unrolling directives for the loop.
On 11/11/2019 14:50, Eric Botcazou wrote: >> Thanks for the explanation. However, I do not understand why are we >> returning with the default value. > > The regression you reported should be clear enough though: if we don't do > that, we will unroll in cases where we would not have before. Try with a > compiler that predates the pragma and compare, there should be no changes. > >> What "do we always do"? > > What we do in the absence of specific unrolling directives for the loop. Yeah fair enough! Sorry for the trouble. Sudi >
diff --git a/gcc/loop-unroll.c b/gcc/loop-unroll.c index 63fccd23fae38f8918a7d94411aaa43c72830dd3..9f7ab4b5c1c9b2333148e452b84afbf040707456 100644 --- a/gcc/loop-unroll.c +++ b/gcc/loop-unroll.c @@ -354,7 +354,7 @@ decide_unroll_constant_iterations (class loop *loop, int flags) widest_int iterations; /* If we were not asked to unroll this loop, just return back silently. */ - if (!(flags & UAP_UNROLL) && !loop->unroll) + if (!(flags & UAP_UNROLL) && loop->unroll == 1) return; if (dump_enabled_p ()) @@ -674,7 +674,7 @@ decide_unroll_runtime_iterations (class loop *loop, int flags) widest_int iterations; /* If we were not asked to unroll this loop, just return back silently. */ - if (!(flags & UAP_UNROLL) && !loop->unroll) + if (!(flags & UAP_UNROLL) && loop->unroll == 1) return; if (dump_enabled_p ()) @@ -1159,7 +1159,7 @@ decide_unroll_stupid (class loop *loop, int flags) widest_int iterations; /* If we were not asked to unroll this loop, just return back silently. */ - if (!(flags & UAP_UNROLL_ALL) && !loop->unroll) + if (!(flags & UAP_UNROLL_ALL) && loop->unroll == 1) return; if (dump_enabled_p ())