Compiler incorrectly assumes int will never be one

[AnttiParaoanu]

The following code tries to parse a decimal integer followed by a newline. For some reason the compiler assumes that the parsed number will never be one. opt-level has to be 2, 3, s or z

fn main() {
    let buffer = [49u8, 10];
    let mut a : u64 = 0;
    'read: loop {
        for c in &buffer {
            match c {
                48..=57 => {
                    a*= 10;
                    a+= *c as u64 - 48;
                }
                10 => {
                    break 'read;
                }
                _ => {
                    unsafe { std::hint::unreachable_unchecked() };
                }
            }
        }
    }
    if a == 1 {
        println!("What did you expect?");
    }
}

I expected to see this happen: When running the program it prints "What did you expect?"

Instead, this happened: The program doesn't print anything, just exits

Meta

rustc --version --verbose:

rustc 1.43.0-nightly (823ff8cf1 2020-03-07)
binary: rustc
commit-hash: 823ff8cf1397a5772b1f6954b60576202bf91836
commit-date: 2020-03-07
host: x86_64-unknown-linux-gnu
release: 1.43.0-nightly
LLVM version: 9.0

On godbolt the issue seems to go back to version 1.38

[ehuss]

Just out of curiosity, I bisected to #62592. Probably not helpful, but I see a note about changes to unreachability in jump tables in the release notes.

[Centril]

cc @nikic @cuviper @alexcrichton

[nikic]

Looks like the condition gets removed by instcombine: https://gist.github.com/nikic/5f13662ae7559c16fdcc2e3cd2b17e5d (Might be caused by a prior change though.)

[cuviper]

I found that the same IR optimizes correctly with LLVM master. I'm currently bisecting...

[nikic]

Based on the structure of the IR, my first guess would be the InstSimplify part of llvm/llvm-project@a7d992c.

[cuviper]

You're right, it's bad up to a7d992c0f2d2^ (1164d43855fd) and correct with a7d992c0f2d2.

[cuviper]

Also, it's fine with LLVM 8, consistent with @ehuss's bisection, so this is just a regression in LLVM 9.

edit: I bisected the regression specifically to llvm/llvm-project@0290a77

[tstellar]

Looks like the condition gets removed by instcombine: https://gist.github.com/nikic/5f13662ae7559c16fdcc2e3cd2b17e5d (Might be caused by a prior change though.)

Could you post dumps from both before and after instcombine ?

[pnkfelix]

@rustbot ping icebreakers-llvm

Namely, we have these LLVM-specific work items:

• The comment thread above indicates that this is fixed upstream
• Is there an attached bug for the fix?
• can we backport a narrow fix?
• and/or push the upgrade through to the newer version of LLVM?

[rustbot]

Hey LLVM ICE-breakers! This bug has been identified as a good
"LLVM ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @comex @DutchGhost @hanna-kruppe @hdhoang @heyrutvik @JOE1994 @jryans @mmilenko @nagisa @nikic @Noah-Kennedy @SiavoshZarrasvand @spastorino @vertexclique @vgxbj

[cuviper]

can we backport a narrow fix?

If this proves difficult, another option is to revert the commit that introduced it.

[nikic]

For backporting, it should be fine to take the part of the patch up to (and including) llvm/llvm-project@a7d992c#diff-1f9547b463bf33c1dd86f485d0fa410eR1465, and drop the part after it. The part below it is a new feature that should not be backported.

[spastorino]

pre-triage: nominating for discussion on next meeting.

[pnkfelix]

I'm volunteering to mentor someone through resolving this bug.

[tesuji]

Doesn't this bug only need a submodule update?

[cuviper]

It needs someone to do the LLVM backport, as @nikic suggested above, in a PR to rust-lang/llvm-project. Then yes, it's a submodule update here, with a regression test if possible.

[pnkfelix]

@nikic , when you wrote:

take the part of the patch up to (and including) llvm/llvm-project@a7d992c#diff-1f9547b463bf33c1dd86f485d0fa410eR1465, and drop the part after it

are the "before"/"after" referring to portions of that diff? Or to commits in a series? (in other words, what is the implicit sequence of things referenced there?)

My current inference is that the "part after it" that you are saying we need to drop the changes that are in static bool isKnownNonNullFromDominatingCondition(..), and keep all the changes that are about the "context instruction to the 'edge' that flows into the phi." but I am not 100% sure about that inference.

[nikic]

My current inference is that the "part after it" that you are saying we need to drop the changes that are in static bool isKnownNonNullFromDominatingCondition(..), and keep all the changes that are about the "context instruction to the 'edge' that flows into the phi." but I am not 100% sure about that inference.

Yup, that's right. The changes to isKnownNonNullFromDominatingCondition() are a new optimization that shouldn't be backported. The rest are fixes for issues that this new optimization exposed, but which are also reproducible (as this issue shows) without it.

[pnkfelix]

Okay, so here are my mentoring notes:

1. Adapt the code on this issue into a regression test. Make sure you have a test that fails. (Note that the issue as written is not a failing test; it just has differing stdout output, which is not really usually how we write our regression tests on this project. E.g. a panic! in the else block would be more in line with how we usually do things.)

2. Grab the diff from llvm/llvm-project@a7d992c#diff-1f9547b463bf33c1dd86f485d0fa410eR1465. I don't know if there's an easy way to coax github into supplying the raw diff for a given commit; but you can go back to the LLVM reviews site for one: https://reviews.llvm.org/file/data/5lxuyx4be3ek64rlbr2w/PHID-FILE-p5gq3z43jepsi32mskks/D71181.diff

3. Extract just the pieces mentioned above, that are about changing the context instruction to the "edge" that flows into the phi, and apply those to the submodule in src/llvm-project.

4. Ensure that the aforementioned change fixes the test you made in step 1.

5. Optional, and probably too much effort to be worthwhile: Figure out how to make a test specific to LLVM project. (One might do this by first taking the original test from step 1 and reducing the LLVM IR associated with it down to something that doesn't rely on anything related to Rust.)

6. Land the LLVM changes on Rust's llvm-project fork.

7. Update Rust's submodule dependency to include the fix on Rust's LLVM fork.

[bjorn3]

don't know if there's an easy way to coax github into supplying the raw diff for a given commit

You can append .diff to the commit url, like so: https://github.com/llvm/llvm-project/commit/a7d992c0f2d235c67f04160405c5c5606408d4b1.diff

[pnkfelix]

(I decided to go ahead and take care of it, further mentoring/publicizing would probably be just as much work as the PR I've posted at rust-lang/llvm-project#44 )

[rustbot]

Hey LLVM ICE-breakers! This bug has been identified as a good
"LLVM ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @comex @cuviper @DutchGhost @hanna-kruppe @hdhoang @heyrutvik @JOE1994 @jryans @mmilenko @nagisa @nikic @Noah-Kennedy @SiavoshZarrasvand @spastorino @vertexclique @vgxbj

[rustbot]

Hey LLVM ICE-breakers! This bug has been identified as a good
"LLVM ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @comex @cuviper @DutchGhost @hanna-kruppe @hdhoang @heyrutvik @JOE1994 @jryans @mmilenko @nagisa @nikic @Noah-Kennedy @SiavoshZarrasvand @spastorino @vertexclique @vgxbj