Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad LLVM code generation on Aarch64 Darwin #39818

Closed
Keno opened this issue Feb 24, 2021 · 4 comments · Fixed by #39891
Closed

Bad LLVM code generation on Aarch64 Darwin #39818

Keno opened this issue Feb 24, 2021 · 4 comments · Fixed by #39891
Labels
system:apple silicon Affects Apple Silicon only (Darwin/ARM64) - e.g. M1 and other M-series chips system:mac Affects only macOS upstream The issue is with an upstream dependency, e.g. LLVM

Comments

@Keno
Copy link
Member

Keno commented Feb 24, 2021

LLVM generates the following code on AArch64 darwin under make debug:

        bl      _japi1_BitSet_5477
        str     x0, [x19, #224]
Lloh112385:
        adrp    x8, __MergedGlobals.2@PAGE
Lloh112386:
        add     x9, x8, __MergedGlobals.2@PAGEOFF
Lloh112387:
        adrp    x8, __MergedGlobals.5@PAGE
Lloh112388:
        add     x8, x8, __MergedGlobals.5@PAGEOFF
        stp     x8, x9, [x19, #24]
        adrp    xzr, __MergedGlobals.1@PAGE
        add     xzr, xzr, __MergedGlobals.1@PAGEOFF

The last statement here is invalid and actually encodes add sp, sp, __MergedGlobals.1@PAGEOFF, which corrupts the stack.

@Keno Keno added system:mac Affects only macOS system:apple silicon Affects Apple Silicon only (Darwin/ARM64) - e.g. M1 and other M-series chips upstream The issue is with an upstream dependency, e.g. LLVM labels Feb 24, 2021
@Keno Keno mentioned this issue Feb 24, 2021
31 tasks
@Keno
Copy link
Member Author

Keno commented Feb 24, 2021

Alright, through some carefully placed assertions, it looks like this comes from the expansion of a MOVaddr psedo-instruction. Those are declared as:

def MOVaddr
    : Pseudo<(outs GPR64:$dst), (ins i64imm:$hi, i64imm:$low),
             [(set GPR64:$dst, (AArch64addlow (AArch64adrp tglobaladdr:$hi),
                                            tglobaladdr:$low))]>,
      Sched<[WriteAdrAdr]>;

However, they expand into the adrp+add seen above, so the GPR64 register class is actually invalid, because that includes xzr which is invalid on the add. The correct register class should be GRP64common. I can submit that patch, but I'm having some trouble coming up with a regression test.

@Keno
Copy link
Member Author

Keno commented Feb 25, 2021

Here's a (bugpoint-reduced) test case that fails on LLVM11, but doesn't fail on master: https://gist.github.com/Keno/a2bb9385fa9b0426df057b9b37652980. I'll bisect LLVM to see if there was an intentional fix of some sort, though I suspect it may just be sensitive to perturbation.

@Keno
Copy link
Member Author

Keno commented Feb 25, 2021

Bisect result:

1cd19fc5681bc2db49dcd2f46a916084357c6e56 is the first new commit
commit 1cd19fc5681bc2db49dcd2f46a916084357c6e56
Author: Andrew Wei <weiwei64@huawei.com>
Date:   Sat Nov 21 00:35:53 2020 +0800

    [DeadMachineInstrctionElim] Post order visit all blocks and Iteratively run DeadMachineInstructionElim pass until nothing dead
    
    Patched by: guopeilin
    Reviewed By: hliao,rampitec
    
    Differential Revision: https://reviews.llvm.org/D91513

 llvm/lib/CodeGen/DeadMachineInstructionElim.cpp | 21 +++++++--
 llvm/test/CodeGen/AArch64/elim-dead-mi.mir      | 61 +++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 5 deletions(-)
 create mode 100644 llvm/test/CodeGen/AArch64/elim-dead-mi.mir

Which makes sense, since this selection could only possibly work if the instruction is dead, so a more aggressive dce would remove it. I'll put up an upstream patch in anyway and also put together an LLVM11 patch for us.

@Keno
Copy link
Member Author

Keno commented Feb 25, 2021

Patch upstream at https://reviews.llvm.org/D97435 even if the web frontend seems broken.

Keno added a commit to JuliaPackaging/Yggdrasil that referenced this issue Mar 1, 2021
Includes patches for
JuliaLang/julia#39823
JuliaLang/julia#39820
JuliaLang/julia#39818
as well as an issue causing an assertion in
debug mode due to address spaces.
Keno added a commit to JuliaPackaging/Yggdrasil that referenced this issue Mar 1, 2021
Includes patches for
JuliaLang/julia#39823
JuliaLang/julia#39820
JuliaLang/julia#39818
as well as an issue causing an assertion in
debug mode due to address spaces.
Keno added a commit to JuliaPackaging/Yggdrasil that referenced this issue Mar 1, 2021
Includes patches for
JuliaLang/julia#39823
JuliaLang/julia#39820
JuliaLang/julia#39818
as well as an issue causing an assertion in
debug mode due to address spaces.
Keno added a commit that referenced this issue Mar 2, 2021
Keno added a commit that referenced this issue Mar 2, 2021
vchuravy pushed a commit that referenced this issue Mar 30, 2021
Fixes #39818
Fixes #39820
Fixes #39823

(cherry picked from commit 0988fcf)
KristofferC pushed a commit that referenced this issue Apr 4, 2021
Fixes #39818
Fixes #39820
Fixes #39823

(cherry picked from commit 0988fcf)
KristofferC pushed a commit that referenced this issue Apr 4, 2021
Fixes #39818
Fixes #39820
Fixes #39823

(cherry picked from commit 0988fcf)
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this issue May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this issue May 9, 2021
vtjnash added a commit to llvm/llvm-project that referenced this issue Sep 9, 2021
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435
vchuravy pushed a commit to JuliaLang/llvm-project that referenced this issue Sep 29, 2021
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435
vchuravy pushed a commit to JuliaLang/llvm-project that referenced this issue Oct 1, 2021
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435
vchuravy pushed a commit to JuliaLang/llvm-project that referenced this issue Oct 9, 2021
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435

(cherry picked from commit 089b115)
github-actions bot pushed a commit to tstellar/llvm-project that referenced this issue Oct 27, 2021
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435

(cherry picked from commit e20f69f)
tstellar pushed a commit to llvm/llvm-project that referenced this issue Nov 10, 2021
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435

(cherry picked from commit e20f69f)
s194604 pushed a commit to s194604/patmos-llvm-project that referenced this issue Apr 10, 2022
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435

(cherry picked from commit e20f69f)
bryanpkc pushed a commit to flang-compiler/classic-flang-llvm-project that referenced this issue Apr 20, 2022
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435

(cherry picked from commit e20f69f)
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
This constrains the Mov* and similar pseudo instruction to take
GPR64common register classes rather than GPR64. GPR64 includs XZR
which is invalid here, because this pseudo instructions expands
into an adrp/add pair sharing a destination register. XZR is invalid
on add and attempting to encode it will instead increment the stack
pointer causing crashes (downstream report at [1]). The test case
there reproduces on LLVM11, but I do not have a test case that
reaches this code path on main, since it is being masked by
improved dead code elimination introduced in D91513. Nevertheless,
this seems like a good thing to fix in case there are other cases
that dead code elimination doesn't clean up (e.g. if `optnone` is
used and the optimization is skipped).

I think it would be worth auditing uses of GPR64 in pseudo
instructions to see if there are any similar issues, but I do not
have a high enough view of the backend or knowledge of the
Aarch64 architecture to do this quickly.

[1] JuliaLang/julia#39818

Reviewed By: t.p.northover

Differential Revision: https://reviews.llvm.org/D97435
staticfloat pushed a commit that referenced this issue Dec 23, 2022
Fixes #39818
Fixes #39820
Fixes #39823

(cherry picked from commit 0988fcf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system:apple silicon Affects Apple Silicon only (Darwin/ARM64) - e.g. M1 and other M-series chips system:mac Affects only macOS upstream The issue is with an upstream dependency, e.g. LLVM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant