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

Miscompile with -Copt-level=1 after update to LLVM 17 #115681

Closed
djkoloski opened this issue Sep 8, 2023 · 5 comments · Fixed by #115959
Closed

Miscompile with -Copt-level=1 after update to LLVM 17 #115681

djkoloski opened this issue Sep 8, 2023 · 5 comments · Fixed by #115959
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@djkoloski
Copy link
Contributor

djkoloski commented Sep 8, 2023

Minimized from code in the Fuchsia tree:

fn main() {
    enum Bits {
        None = 0x00,
        Low = 0x40,
        High = 0x80,
        Both = 0xC0,
    }

    let value = Box::new(0x40u8);
    let mut out = Box::new(0u8);

    let bits = match *value {
        0x00 => Bits::None,
        0x40 => Bits::Low,
        0x80 => Bits::High,
        0xC0 => Bits::Both,
        _ => return,
    };

    match bits {
        Bits::None | Bits::Low => {
            *out = 1;
        }
        _ => (),
    }

    assert_eq!(*out, 1);
}

This issue was introduced in the nightly-2023-08-09 build, and so it is likely to be a result of the upgrade to LLVM 17 (#114048).

$ RUSTFLAGS="-Copt-level=1" cargo +nightly-2032-08-09 run
thread 'main' panicked at src/main.rs:27:5:
assertion `left == right` failed
  left: 0
 right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
$ RUSTFLAGS="-Copt-level=1" cargo +nightly-2032-08-08 run
<passes>
@djkoloski djkoloski added the C-bug Category: This is a bug. label Sep 8, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 8, 2023
@djkoloski djkoloski changed the title Missing branch in ASAN builds on nightly Miscompile with -Copt-level=1 on nightly Sep 11, 2023
@djkoloski djkoloski changed the title Miscompile with -Copt-level=1 on nightly Miscompile with -Copt-level=1 after update to LLVM 17 Sep 11, 2023
@djkoloski
Copy link
Contributor Author

I have bisected the LLVM pass to the instcombine pass on main. I'm trying to get a diff of before/after so I can start tracking down the exact location of the incorrect optimization.

@tmiasko tmiasko added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Sep 11, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 11, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 11, 2023
@tmiasko tmiasko added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Sep 11, 2023
@djkoloski
Copy link
Contributor Author

The optimization transforms this switch lookup which appears to correspond to the match:

switch.lookup:                                    ; preds = %"_ZN5alloc5boxed12Box$LT$T$GT$3new17h3cb0a1b6f587dddeE.exit4"
  %switch.idx.mult = mul nsw i8 %8, 64, !dbg !682
  %switch.offset = add nsw i8 %switch.idx.mult, -128, !dbg !682
  call void @llvm.dbg.value(metadata i64 poison, metadata !531, metadata !DIExpression()), !dbg !719
  switch i8 %switch.offset, label %bb10 [
    i8 0, label %bb9
    i8 64, label %bb9
  ], !dbg !720

with only the calculation of %switch.idx.mult and %switch.offset changing from:

%switch.idx.mult = mul nsw i8 %8, 64, !dbg !682
%switch.offset = add nsw i8 %switch.idx.mult, -128, !dbg !682

to:

%switch.idx.mult = shl nsw i8 %8, 6, !dbg !682
%switch.offset = or i8 %switch.idx.mult, -128, !dbg !682

with the rest of the IR remaining unchanged.

The transform of %switch.idx.mult appears to be okay (mul by 64 -> shl by 6). The transform of %switch.offset introduces the issue because although the add instruction is marked as nsw, %switch.idx.mult is equal to 192 which is negative in the signed representation and so subtracting 128 does cause signed underflow.

@tmandry tmandry added P-critical Critical priority A-codegen Area: Code generation WG-llvm Working group: LLVM backend code generation and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. WG-llvm Working group: LLVM backend code generation labels Sep 11, 2023
@djkoloski
Copy link
Contributor Author

The IR output from rustc without any optimization passes outputs is:

switch i8 %8, label %bb3 [
  i8 0, label %bb4
  i8 64, label %bb5
  i8 -128, label %bb6
  i8 -64, label %bb7
], !dbg !1471

Which appears correct to me. This suggests that the add nsw is being introduced in a later optimization pass. I'll continue bisecting when I get a chance.

@djkoloski
Copy link
Contributor Author

djkoloski commented Sep 11, 2023

Looks like this add nsw instruction is being generated by a simplifycfg<switch-to-lookup> pass that's run before the instcombine pass.

Before simplifycfg:

switch i8 %8, label %bb3 [
  i8 2, label %bb8
  i8 3, label %bb5
  i8 0, label %bb6
  i8 1, label %bb7
], !dbg !682

Which is correct. So the real errant pass is simplifycfg<switch-to-lookup>, it just doesn't cause incorrect x86 to be emitted until a later pass.

@djkoloski
Copy link
Contributor Author

Looks like this issue is known and a fix is in progress upstream at llvm/llvm-project#65882.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2023
Update to LLVM 17.0.0

This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2023
Update to LLVM 17.0.0

This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 20, 2023
Update to LLVM 17.0.0

This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.
@bors bors closed this as completed in 531830c Sep 20, 2023
cuviper pushed a commit to cuviper/rust that referenced this issue Sep 22, 2023
This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.

(cherry picked from commit 531830c)
xobs pushed a commit to betrusted-io/rust that referenced this issue Oct 3, 2023
This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.

(cherry picked from commit 531830c)
plietar pushed a commit to plietar/rust that referenced this issue Oct 18, 2023
This rebases our LLVM fork to 17.0.0.

Fixes rust-lang#115681.

(cherry picked from commit 531830c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants