Hi,
Thanks for the patch.
Omar Tahir <omar.ta...@arm.com> writes:
> Hi,
>
> The variables first_moveable_pseudo and last_moveable_pseudo aren't reset
> after compiling a function, which means they leak into the first scheduler
> pass of the following function. In some cases, this can cause an extra spill
> during register allocation of the second function.
>
> This patch fixes this by setting
>
> first_moveable_pseudo = last_moveable_pseudo
>
> at the beginning of the first scheduler pass.
>
> Because the spill occurs in the middle of the IRA pass it is highly
> target-sensitive, so I have provided a test case that works on aarch64. There
> doesn't seem to be a target-independent way to trigger this bug, since the
> RTL at the IRA stage is already quite target-specific.
>
> Interestingly, the aarch64 test case here doesn't actually perform a complete
> register spill - the value is stored on the stack but never retrieved.
> Perhaps this points to another, deeper bug?
>
> Bootstrapped and regression tested on aarch64-none-linux-gnu.
>
> I don't have write privileges, so if it's fine could someone push for me?
>
> Thanks,
> Omar
>
> ----
>
> gcc/ChangeLog:
>
> 2020-06-30: Omar Tahir <omar.ta...@arm.com>
>
> * sched-rgn.c: Include ira-int.h, ira.h, regs.h.
> (rest_of_handle_sched): Clear moveable pseudo range.
>
> gcc/testsuite/ChangeLog:
>
> 2020-06-30: Omar Tahir <omar.ta...@arm.com>
>
> * gcc.target/aarch64/nospill.c: New test.
>
> ----
>
> diff --git a/gcc/sched-rgn.c b/gcc/sched-rgn.c
> index 7f5dfdb..51329fc 100644
> --- a/gcc/sched-rgn.c
> +++ b/gcc/sched-rgn.c
> @@ -65,6 +65,9 @@ along with GCC; see the file COPYING3. If not see
> #include "dbgcnt.h"
> #include "pretty-print.h"
> #include "print-rtl.h"
> +#include "ira.h"
> +#include "regs.h"
> +#include "ira-int.h"
>
> /* Disable warnings about quoting issues in the pp_xxx calls below
> that (intentionally) don't follow GCC diagnostic conventions. */
> @@ -3719,6 +3722,7 @@ static unsigned int
> rest_of_handle_sched (void)
> {
> #ifdef INSN_SCHEDULING
> + first_moveable_pseudo = last_moveable_pseudo;
> if (flag_selective_scheduling
> && ! maybe_skip_selective_scheduling ())
> run_selective_scheduling ();
I think instead we should zero out both variables at the end of IRA.
There are other places besides the scheduler that call into the IRA code,
so tackling the problem there seems more general.
> diff --git a/gcc/testsuite/gcc.target/aarch64/nospill.c
> b/gcc/testsuite/gcc.target/aarch64/nospill.c
> new file mode 100644
> index 0000000..60399d8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/nospill.c
> @@ -0,0 +1,35 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3" } */
> +
> +/* The pseudo for P is marked as moveable in the IRA pass. */
> +float
> +func_0 (float a, float b, float c)
> +{
> + float p = c / a;
> +
> + if (b > 1)
> + {
> + b /= p;
> + if (c > 2)
> + a /= 3;
> + }
> +
> + return b / c * a;
> +}
> +
> +/* If first_moveable_pseudo and last_moveable_pseudo are not reset correctly,
> + they will carry over and spill the pseudo for Q. */
> +float
> +func_1 (float a, float b, float c)
> +{
> + float q = a + b;
> +
> + c *= a / (b + b);
> + if (a > 0)
> + c *= q;
> +
> + return a * b * c;
> +}
> +
> +/* We have plenty of spare registers, so check nothing has been spilled. */
> +/* { dg-final { scan-assembler-not "str" } } */
The testcase looks good, but it's probably better to make that “\tstr\t”.
The problem with plain “str” is that assembly output can often include
pathnames and version strings, and it's possible that one of those could
contain “str”.
Thanks,
Richard
