Alexander Monakov <amona...@ispras.ru> writes:
> On Fri, 23 Dec 2022, Jose E. Marchesi wrote:
>
>> > +1 for trying this FWIW. There's still plenty of time to try an
>> > alternative solution if there are unexpected performance problems.
>>
>> Let me see if Alexander's patch fixes the issue at hand (it must) and
>> will also do some regression testing.
>
> Hi, I'm not sure at which court the ball is, but in the interest at moving
> things forward here's the complete patch with the testcase. OK to apply?
>
> ---8<---
>
> From: Alexander Monakov <amona...@ispras.ru>
> Date: Fri, 13 Jan 2023 21:04:02 +0300
> Subject: [PATCH] sched-deps: do not schedule pseudos across calls [PR108117]
>
> Scheduling across calls in the pre-RA scheduler is problematic: we do
> not take liveness info into account, and are thus prone to extending
> lifetime of a pseudo over the loop, requiring a callee-saved hardreg
> or causing a spill.
>
> If current function called a setjmp, lifting an assignment over a call
> may be incorrect if a longjmp would happen before the assignment.
>
> Thanks to Jose Marchesi for testing on AArch64.
>
> gcc/ChangeLog:
>
> PR rtl-optimization/108117
> PR rtl-optimization/108132
> * sched-deps.cc (deps_analyze_insn): Do not schedule across
> calls before reload.
>
> gcc/testsuite/ChangeLog:
>
> PR rtl-optimization/108117
> PR rtl-optimization/108132
> * gcc.dg/pr108117.c: New test.
OK, thanks.
Richard
> ---
> gcc/sched-deps.cc | 9 ++++++++-
> gcc/testsuite/gcc.dg/pr108117.c | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr108117.c
>
> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
> index 948aa0c3b..5dc4fa4cd 100644
> --- a/gcc/sched-deps.cc
> +++ b/gcc/sched-deps.cc
> @@ -3688,7 +3688,14 @@ deps_analyze_insn (class deps_desc *deps, rtx_insn
> *insn)
>
> CANT_MOVE (insn) = 1;
>
> - if (find_reg_note (insn, REG_SETJMP, NULL))
> + if (!reload_completed)
> + {
> + /* Scheduling across calls may increase register pressure by extending
> + live ranges of pseudos over the call. Worse, in presence of setjmp
> + it may incorrectly move up an assignment over a longjmp. */
> + reg_pending_barrier = MOVE_BARRIER;
> + }
> + else if (find_reg_note (insn, REG_SETJMP, NULL))
> {
> /* This is setjmp. Assume that all registers, not just
> hard registers, may be clobbered by this call. */
> diff --git a/gcc/testsuite/gcc.dg/pr108117.c b/gcc/testsuite/gcc.dg/pr108117.c
> new file mode 100644
> index 000000000..ae151693e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr108117.c
> @@ -0,0 +1,30 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target nonlocal_goto } */
> +/* { dg-options "-O2 -fschedule-insns" } */
> +
> +#include <stdio.h>
> +#include <setjmp.h>
> +
> +jmp_buf ex_buf;
> +
> +__attribute__((noipa))
> +void fn_throw(int x)
> +{
> + if (x)
> + longjmp(ex_buf, 1);
> +}
> +
> +int main(void)
> +{
> + int vb = 0; // NB: not volatile, not modified after setjmp
> +
> + if (!setjmp(ex_buf)) {
> + fn_throw(1);
> + vb = 1; // not reached in the abstract machine
> + }
> +
> + if (vb) {
> + printf("Failed, vb = %d!\n", vb);
> + return 1;
> + }
> +}
