optimized i686 fails primal-sieve tests

[cuviper]

Discovered in huonw/primal#37 (comment)

Code

I don't have reduced code, but the primal-sieve testsuite fails on i686-unknown-linux-gnu when compiled in --release mode, specifically on this line:
https://github.com/huonw/primal/blob/79bafd3400ecf530bea1d26db4f3854407588c2a/primal-sieve/src/sieve.rs#L769

    #[test]
    fn sum_primes() {
        let primes = Sieve::new(2_000_000);

        let mut manual_sum = 0u64;
        for p in primes.primes_from(0) {
            manual_sum += p as u64;
        }
        dbg!(manual_sum);

        let folded_sum = primes.primes_from(0).fold(0u64, |acc, p| acc + p as u64);
        let trait_sum = primes.primes_from(0).map(|p| p as u64).sum::<u64>();
        assert_eq!(manual_sum, folded_sum); // <-- FAILED
        assert_eq!(manual_sum, trait_sum);
    }

It fails when compiled from x86_64 host:

$ cd primal-sieve
$ cargo +nightly test --release --target i686-unknown-linux-gnu sum_primes

or with a native i686 host toolchain:

$cargo +nightly-i686 test --release sum_primes

I expected to see this happen: the test should pass

Instead, this happened: FAILED

---- sieve::tests::sum_primes stdout ----
[primal-sieve/src/sieve.rs:765] manual_sum = 307525716868
thread 'sieve::tests::sum_primes' panicked at 'assertion failed: `(left == right)`
  left: `307525716868`,
 right: `142913828922`', primal-sieve/src/sieve.rs:769:9

The left manual_sum is wrong, and the right folded_sum is correct. They both come from the same generated primes sieve, so that part of the code appears to have worked correctly. That leaves the difference between the for loop and the fold, and while SievePrimes does implement both next and fold, the differences are small.

The same test passes on i686 in debug mode, as well as x86_64 in debug or release.

Version it worked on

It most recently worked on: stable Rust 1.58.1

Version with regression

Both beta and nightly are failing the same way.

rustc 1.59.0-beta.8 (1945ce657 2022-02-12)
binary: rustc
commit-hash: 1945ce6579506787e0b18f0a2ea03fdb4dfc81c7
commit-date: 2022-02-12
host: x86_64-unknown-linux-gnu
release: 1.59.0-beta.8
LLVM version: 13.0.0

rustc 1.60.0-nightly (c5c610aad 2022-02-14)
binary: rustc
commit-hash: c5c610aad0a012a9228ecb83cc19e77111a52140
commit-date: 2022-02-14
host: x86_64-unknown-linux-gnu
release: 1.60.0-nightly
LLVM version: 13.0.0

@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

[cuviper]

I just tried armv7-unknown-linux-gnueabihf under qemu to see if the bug is a 32-bit thing, but that passes fine.

[cuviper]

It passes on i686 release with -Znew-llvm-pass-manager=no.

[nikic]

The relevant difference with the NewPM seems to be that the next() method gets inlined now.

[apiraino]

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-critical

[nikic]

Just tested this with the 9f2b322c3782ddda66efcfa36900d27f4507707e toolchain and it still fails, so this issue is not fixed in LLVM 14.

[cuviper]

SievePrimes::next() does have an #[inline] attribute, and if I remove that, the test passes. But even #[inline(always)] is not enough to make oldPM fail.

Also, forcing newPM with 1.58.1 still passes as well! Between stable and beta, the most significant change looks like the patch to disable deferred inlining -- and the test does pass on beta and nightly with -Cllvm-args=-inline-deferral=1.

(That doesn't mean the inlining decision is wrong -- it's probably just creating circumstances that are buggy in another pass.)

[pnkfelix]

@cuviper and I just found a slight variant that shows a similar failure on x86_64....

[cuviper]

We're discussing in this Zulip thread. After discovering the variant where x86_64 fails, we found that it also failed on stable back to 1.55, but passed on 1.54. Bisecting between those took me to #84560, a simple #[inline] addition, which I take to mean it just perturbed the code enough to expose a pre-existing codegen bug.

So, I'm removing the regression label, and we can treat it like any ongoing bug.

[pnkfelix]

FYI I took the test from the primal crate and reduced it down to a single (large!) file: https://gist.github.com/pnkfelix/f45098bc69482dac920dcf384049b849

I suspect this is still too absurdly large to turn into LLVM bitcode to file a bug with LLVM. But, just as i write this, I realize it might suffice as input for the bitcode automatic reduction tools that LLVM distributes. Looking in that next.

[pnkfelix]

This is flagged as P-critical, but I do not believe it is a 1.59.0 release blocker. (And I think the release team agrees with this.)

The investigation, covered in zulip topic, determined this bug is visible as far back as 1.55.0.

So: should it be downgraded to P-high? I think the nature of the bug (outright miscompilation) warrants us covering it on a weekly basis in T-compiler meeting, at least for now. So I won't downgrade it yet; we can let the team decide if it should be downgraded during regular triage meeting.

[nikic]

Upstream issue: llvm/llvm-project#53990 Not a great reduction, but my attempts to construct a minimal MIR test failed.

[cuviper]

The proposed patch does work for me, thanks!

[apiraino]

Priority label has been mentioned during T-compiler weekly meeting on Zulip. This will be reviewed in a follow-up meeting.