Fix unrolling check.
diff mbox series

Message ID 3e964005-5e24-0f15-0c88-b833e086debf@arm.com
State New
Headers show
Series
  • Fix unrolling check.
Related show

Commit Message

Sudakshina Das Nov. 8, 2019, 4:54 p.m. UTC
Hi

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.

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.

Does this change look ok? Can I just remove the -fopt-info from the test 
or unrolling the loop in the test is not desirable?

Thanks
Sudi

gcc/ChangeLog:

2019-11-07  Sudakshina Das  <sudi.das@arm.com>

	* loop-unroll.c (decide_unroll_constant_iterations): Update condition 
to check
	loop->unroll.
	(decide_unroll_runtime_iterations): Likewise.
	(decide_unroll_stupid): Likewise.

Comments

Eric Botcazou Nov. 8, 2019, 7:16 p.m. UTC | #1
> 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.
Sudakshina Das Nov. 11, 2019, 11:38 a.m. UTC | #2
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.
>
Eric Botcazou Nov. 11, 2019, 2:50 p.m. UTC | #3
> 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.
Sudakshina Das Nov. 11, 2019, 5:40 p.m. UTC | #4
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
>

Patch
diff mbox series

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 ())