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

[SimplifyCFG] Need to remove useless comparison after turning switch to icmp #53853

Closed
wweiandrew opened this issue Feb 15, 2022 · 0 comments
Closed

Comments

@wweiandrew
Copy link
Contributor

For example,

define i64 @demo(i64 %x) {
entry:
    switch i64 %x, label %bb3 [
        i64 0, label %bb1
        i64 1, label %bb2
    ]
bb1:
    ret i64 0
bb2:
    %0 = icmp eq i64 %x, 100 ; this will necessarily be false because %x == 1
    br i1 %0, label %bb4, label %bb5
bb3:
    unreachable
bb4:
    ret i64 200
bb5:
    ret i64 10
}

See: https://godbolt.org/z/3nobrzoKa
This is a known issue from rust: rust-lang/rust#85133 (comment)

We want to get the IR result after SimplifyCFG:

define i64 @src(i64 %x) {
%entry:
  %switch = icmp eq i64 %x, 0
  %. = select i1 %switch, i64 0, i64 10
  ret i64 %.
}
LebedevRI pushed a commit that referenced this issue Feb 15, 2022
Based on original tests from D119839.

See #53853
LebedevRI pushed a commit to LebedevRI/llvm-project that referenced this issue Feb 23, 2022
Based on original tests from D119839.

See llvm#53853
LebedevRI added a commit to LebedevRI/llvm-project that referenced this issue Feb 23, 2022
…nge comparison and branch until after at least the IPSCCP

That transformation is lossy, as discussed in
llvm#53853
and rust-lang/rust#85133 (comment)

This is an alternative to D119839,
which would add a limited IPSCCP into SimplifyCFG.

Unlike lowering switch to lookup, we still want this transformation
to happen relatively early, but after giving a chance for the things
like CVP to do their thing. It seems like deferring it just until
the IPSCCP is enough for the tests at hand, but perhaps we need to
be more aggressive and disable it until CVP.

Fixes llvm#53853
Refs. rust-lang/rust#85133

Reviewed By: nikic

Differential Revision: https://reviews.llvm.org/D119854
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 24, 2022
Based on original tests from D119839.

See llvm#53853

(cherry picked from commit c9c9307)
nikic pushed a commit to nikic/llvm-project that referenced this issue Mar 2, 2022
Based on original tests from D119839.

See llvm#53853

(cherry picked from commit c9c9307)
nikic pushed a commit to nikic/llvm-project that referenced this issue Mar 2, 2022
…nge comparison and branch until after at least the IPSCCP

That transformation is lossy, as discussed in
llvm#53853
and rust-lang/rust#85133 (comment)

This is an alternative to D119839,
which would add a limited IPSCCP into SimplifyCFG.

Unlike lowering switch to lookup, we still want this transformation
to happen relatively early, but after giving a chance for the things
like CVP to do their thing. It seems like deferring it just until
the IPSCCP is enough for the tests at hand, but perhaps we need to
be more aggressive and disable it until CVP.

Fixes llvm#53853
Refs. rust-lang/rust#85133

Reviewed By: nikic

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

(cherry picked from commit 371fcb7)
tstellar pushed a commit to llvmbot/llvm-project that referenced this issue Mar 9, 2022
Based on original tests from D119839.

See llvm#53853
tstellar pushed a commit to llvmbot/llvm-project that referenced this issue Mar 9, 2022
…nge comparison and branch until after at least the IPSCCP

That transformation is lossy, as discussed in
llvm#53853
and rust-lang/rust#85133 (comment)

This is an alternative to D119839,
which would add a limited IPSCCP into SimplifyCFG.

Unlike lowering switch to lookup, we still want this transformation
to happen relatively early, but after giving a chance for the things
like CVP to do their thing. It seems like deferring it just until
the IPSCCP is enough for the tests at hand, but perhaps we need to
be more aggressive and disable it until CVP.

Fixes llvm#53853
Refs. rust-lang/rust#85133

Reviewed By: nikic

Differential Revision: https://reviews.llvm.org/D119854
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
…nge comparison and branch until after at least the IPSCCP

That transformation is lossy, as discussed in
llvm/llvm-project#53853
and rust-lang/rust#85133 (comment)

This is an alternative to D119839,
which would add a limited IPSCCP into SimplifyCFG.

Unlike lowering switch to lookup, we still want this transformation
to happen relatively early, but after giving a chance for the things
like CVP to do their thing. It seems like deferring it just until
the IPSCCP is enough for the tests at hand, but perhaps we need to
be more aggressive and disable it until CVP.

Fixes llvm/llvm-project#53853
Refs. rust-lang/rust#85133

Reviewed By: nikic

Differential Revision: https://reviews.llvm.org/D119854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant