Skip to content

Possible regression for ui/match/issue-114691.rs #141340

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

Open
lewis1revill opened this issue May 21, 2025 · 4 comments
Open

Possible regression for ui/match/issue-114691.rs #141340

lewis1revill opened this issue May 21, 2025 · 4 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lewis1revill
Copy link

Copied code for the test in question here:

//@ run-pass

// This test used to be miscompiled by LLVM 17.
#![allow(dead_code)]

enum Pass {
    Opaque {
        clear_color: [f32; 4],
        with_depth_pre_pass: bool,
    },
    Transparent,
}

enum LoadOp {
    Clear,
    Load,
}

#[inline(never)]
fn check(x: Option<LoadOp>) {
    assert!(x.is_none());
}

#[inline(never)]
fn test(mode: Pass) {
    check(match mode {
        Pass::Opaque {
            with_depth_pre_pass: true,
            ..
        }
        | Pass::Transparent => None,
        _ => Some(LoadOp::Clear),
    });
}

fn main() {
    println!("Hello, world!");
    test(Pass::Transparent);
}

This test is regressing for us with a custom target, because the problematic transformation foldBoolSelectToLogic still runs on the IR for fn test. This is after updating to LLVM 19.

To me, it's clear to see why the transformation is invalid, but for seemingly different reasons than the above fix addresses. The IR is:

; main::test
; Function Attrs: noinline nounwind
define internal fastcc void @_ZN4main4test17h24436c5bdc3836e4E(i8 %mode.0.val, i8 %mode.1.val) unnamed_addr #2 {
start:
  %trunc = trunc nuw i8 %mode.0.val to i1
  %0 = trunc nuw i8 %mode.1.val to i1
  %1 = select i1 %trunc, i1 true, i1 %0
  %.sink = select i1 %1, i8 2, i8 0
; call main::check
  tail call fastcc void @_ZN4main5check17h13ab3f87dc46addbE(i8 noundef %.sink) #5
  ret void
}

Which represents a logical, short-circuiting or operation on %mode.0.val and %mode.1.val. And since %mode.1.val is not marked as noundef, it shouldn't be valid to transform that operation into a bitwise or, since %mode.1.val being undefined now has an effect on the result regardless of whether %mode.0.val is true or not.

Does anyone have a theory as to why this fix used to work, but seems to no longer work? Frustratingly, the testcase still passes for x86_64, but that doesn't mean anything about this fix because they have switched to using GlobalISel.

Relates to issue #114691

@lewis1revill lewis1revill added the C-bug Category: This is a bug. label May 21, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 21, 2025
@lolbinarycat lolbinarycat added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-untriaged Untriaged performance or correctness regression. labels May 22, 2025
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 22, 2025
@apiraino
Copy link
Contributor

apiraino commented May 26, 2025

@lewis1revill what is the custom target you're mentioning? Can we reproduce the issue? Can you help bisecting with cargo-bisect-rustc?

For easier linking, we updated to LLVM 19 in #127513. I infer this is when that test started regressing for you?

@rustbot label E-needs-bisection

@rustbot rustbot added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label May 26, 2025
@nikic
Copy link
Contributor

nikic commented May 26, 2025

Can you try using LLVM 20 instead? LLVM 20 fixed the underlying issue. It's possible that the previous workaround in prior versions is insufficient if trunc nuw is used.

@jieyouxu jieyouxu added the S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. label May 26, 2025
@workingjubilee
Copy link
Member

"they" have switched to using GlobalISel? Oh, LLVM has switched to using GlobalISel with x86 by default? neat?

@nikic
Copy link
Contributor

nikic commented May 26, 2025

"they" have switched to using GlobalISel? Oh, LLVM has switched to using GlobalISel with x86 by default? neat?

It hasn't.

@Noratrieb Noratrieb added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants