[Bug rtl-optimization/108117] New: Wrong instruction scheduling on value coming from abnormal SSA

[Bug rtl-optimization/108117] New: Wrong instruction scheduling on value coming from abnormal SSA

[fxue at os dot amperecomputing.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

            Bug ID: 108117
           Summary: Wrong instruction scheduling on value coming from
                    abnormal SSA
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: fxue at os dot amperecomputing.com
  Target Milestone: ---

Compile the following code with "-O2" on AArch64.

#include <stdio.h>
#include <setjmp.h>

jmp_buf ex_buf;

__attribute__((noinline)) int fn_throw(int x)
{
   if (x == 1)
      longjmp(ex_buf, 1);
   return 1;
}

int main(int argc, char** argv)
{
    int va = 0;
    int vb = 0;

    if (!setjmp(ex_buf)) {
        va = fn_throw(1); /* throw via longjmp */
        vb = 1;
    } else
        printf("Got exception, va = %d\n", va);

    if (vb)
        printf("Failed, vb should not = %d!\n", vb);

    return 0;
}

Since "fn_throw" involves abnormal control flow transferring, any statement
after "va = fn_throw(1)" should not be hoisted prior to the call. In this case,
"vb = 1" is moved before it by RTL inst sched, and leads to incorrect result.

Similar to C++ exception handling, setjmp/longjmp would generate SSA names
occurring in abnormal phi, these should be specially treated. Though, it looks
like that RTL passes do not respect this characteristics in any kind of code
motions. Now the issue is only exposed on AArch64, for while inst sched1 pass
is enabled, but it is also a potential one on other backends.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Vb needs to be marked as volatile.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
https://en.cppreference.com/w/c/program/setjmp

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There used to be a warning for this. Even saw someone post an updated version
of it that happens on gimple (but that never went in).

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amonakov at gcc dot gnu.org

--- Comment #4 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
-Wclobbered properly warns here (and it's part of -Wextra).

With explicit -fschedule-insns, reproducible on x86 as well.

The reason for the issue is quite surprising though, I did not expect pre-RA
scheduling to lift assignments to pseudos across calls, because it just
increases register pressure at the point of the call for little or no gain.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |---

--- Comment #5 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
On further thought, this is really an invalid transform, because the value
becomes "clobbered" only if it was changed between setjmp and longjmp:

(C11 7.13.2.1 "The longjmp function")
>  All accessible objects have values, and all other components of the abstract
> machine have state, as of the time the longjmp function was called, except that
> the values of objects of automatic storage duration that are local to the
> function containing the invocation of the corresponding setjmp macro that
> do not have volatile-qualified type and have been changed between the setjmp
> invocation and longjmp call are indeterminate.

In the testcase, the assignment 'vb = 1' did not happen in the abstract
machine.

Moving back to UNCONFIRMED, both because the transform is invalid, and because
lifting assignments to pseudos across calls in sched1 seems useless if not
harmful to performance and code size.

(that said, the -Wclobbered diagnostic still points to a potential issue, so it
shouldn't be ignored)

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[fxue at os dot amperecomputing.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #6 from Feng Xue <fxue at os dot amperecomputing.com> ---
(In reply to Andrew Pinski from comment #2)
> https://en.cppreference.com/w/c/program/setjmp

I think that most programmers are not aware of this, neither I for sure. Usage
of volatile here is not that intuitive as for purpose of multi-execution
synchronization, it just tells compiler to keep variable in memory, instead of
register. With current SSA representation in GCC, probably, this could be
automatically identified without explicit source level specifier. That is, any
SSA name occurring in abnormal phi should go to memory during expansion to rtl.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Feng Xue from comment #6)
> (In reply to Andrew Pinski from comment #2)
> > https://en.cppreference.com/w/c/program/setjmp
> 
> I think that most programmers are not aware of this, neither I for sure.
> Usage of volatile here is not that intuitive as for purpose of
> multi-execution synchronization

You should read up on https://en.cppreference.com/w/cpp/utility/program/signal
too.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[fxue at os dot amperecomputing.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #8 from Feng Xue <fxue at os dot amperecomputing.com> ---
(In reply to Andrew Pinski from comment #7)
> (In reply to Feng Xue from comment #6)
> > (In reply to Andrew Pinski from comment #2)
> > > https://en.cppreference.com/w/c/program/setjmp
> > 
> > I think that most programmers are not aware of this, neither I for sure.
> > Usage of volatile here is not that intuitive as for purpose of
> > multi-execution synchronization
> 
> You should read up on
> https://en.cppreference.com/w/cpp/utility/program/signal too.

From viewpoint of programmer, setjmp/longjmp is somewhat similar to C++
exception handling, which happens in one logical execution context, while
signal implies two  unrelated contexts.

In another angle, because gcc already model control flow and SSA web for
setjmp/longjmp, explicit volatile specification is not really needed. But
signal mechanism is kind of asynchronous exception, for which it is impractical
for compiler to do that, so volatile is must.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #9 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
(In reply to Feng Xue from comment #8)

> In another angle, because gcc already model control flow and SSA web for
> setjmp/longjmp, explicit volatile specification is not really needed.

That covers GIMPLE, but after transitioning to RTL, setjmp is not properly
modeled anymore (like in old versions of GCC before Tree-SSA). Many RTL passes
simply refuse touching the function if it has a setjmp call, but as your
example demonstrated, scheduling still can make a surprising transform.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |DUPLICATE

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Dup of bug 57067.

*** This bug has been marked as a duplicate of bug 57067 ***

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
From the duplicated bug: "In this case the RTL scheduler pass generates broken
code due to the missing control flow info."

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #12 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Shouldn't there be another bug for the sched1 issue specifically? In absence of
abnormal control flow, extending lifetimes of pseudos across calls is still
likely to be a pessimization.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Alexander Monakov from comment #12)
> Shouldn't there be another bug for the sched1 issue specifically? In absence
> of abnormal control flow, extending lifetimes of pseudos across calls is
> still likely to be a pessimization.

The lifetime of the pseduo was already across the call ...

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|DUPLICATE                   |FIXED

--- Comment #14 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #13)
> 
> The lifetime of the pseduo was already across the call ...

Hm, I disagree: 'vb = 1' is a killing definition. Therefore the 'vb = 0'
initialization is dead at the point of the call.

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |DUPLICATE

--- Comment #15 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Sorry, didn't mean to remove the duplicate info. I could swear I didn't touch
the dropdown, not sure what happened.

*** This bug has been marked as a duplicate of bug 57067 ***

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #16 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Draft patch for the sched1 issue:
https://inbox.sourceware.org/gcc-patches/cf62c3ec-0a9e-275e-5efa-2689ff1f00e2@ispras.ru/T/#m95238afa0f92daa0ba7f8651741089e7cfc03481

[Bug rtl-optimization/108117] Wrong instruction scheduling on value coming from abnormal SSA

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108117

--- Comment #17 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Alexander Monakov <amonakov@gcc.gnu.org>:

https://gcc.gnu.org/g:733a1b777f16cd397b43a242d9c31761f66d3da8

commit r13-5154-g733a1b777f16cd397b43a242d9c31761f66d3da8
Author: Alexander Monakov <amonakov@ispras.ru>
Date:   Fri Jan 13 21:04:02 2023 +0300

    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.