-
Notifications
You must be signed in to change notification settings - Fork 12.7k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[InstCombine] Fold Ext(i1) Pred shr(A, BW - 1) => i1 Pred A s< 0 (#68244
) Resolves #67916 . This patch folds `Ext(icmp (A, xxx)) Pred shr(A, BW - 1)` into `i1 Pred A s< 0`. [Alive2](https://alive2.llvm.org/ce/z/k53Xwa).
- llvmorg-21-init
- llvmorg-20.1.0-rc2
- llvmorg-20.1.0-rc1
- llvmorg-20-init
- llvmorg-19.1.7
- llvmorg-19.1.6
- llvmorg-19.1.5
- llvmorg-19.1.4
- llvmorg-19.1.3
- llvmorg-19.1.2
- llvmorg-19.1.1
- llvmorg-19.1.0
- llvmorg-19.1.0-rc4
- llvmorg-19.1.0-rc3
- llvmorg-19.1.0-rc2
- llvmorg-19.1.0-rc1
- llvmorg-19-init
- llvmorg-18.1.8
- llvmorg-18.1.7
- llvmorg-18.1.6
- llvmorg-18.1.5
- llvmorg-18.1.4
- llvmorg-18.1.3
- llvmorg-18.1.2
- llvmorg-18.1.1
- llvmorg-18.1.0
- llvmorg-18.1.0-rc4
- llvmorg-18.1.0-rc3
- llvmorg-18.1.0-rc2
- llvmorg-18.1.0-rc1
Showing
3 changed files
with
249 additions
and
72 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit breaks some of our code. An example on compiler explorer that shows a change in the behavior: https://gcc.godbolt.org/z/axb17MYMG
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference in the produced IR caused by this patch is:
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A self-contained test case without external headers: https://gcc.godbolt.org/z/51cscMc7e
The corresponding IR: https://gcc.godbolt.org/z/f9zWTh4Pa
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XChy Please take a look!
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm investigating it now.
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the proof for the correctness of the transform(src is clang17.0.0, tgt is trunk). This includes all changes (except main) of your link at IR level.
https://alive2.llvm.org/ce/z/B66Wha
https://alive2.llvm.org/ce/z/PMmyRj
For changes in main, Alive2 cannot handle lifetime currently. But I don't think it's a correct transform, which eliminates overflow check.
What confuse me is that
opt trunk
did well here, https://gcc.godbolt.org/z/nojq9qT6K.Maybe this commit exposes the bug of other optimization.
You could post a issue about it and feel free to revert this commit.
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problematic transform is this one: https://alive2.llvm.org/ce/z/XUTETD alive2 says it's correct (even if I add
-src-unroll
and-tgt-unroll
), but I don't get why it would be correct to remove the overflow check there...b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also a problem of Alive2? I think we should also post an issue on Alive2's repo.
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened an alive2 issue here: AliveToolkit/alive2#951
I think the JumpThreading problem is that computeValueKnownInPredecessorsImpl() will perform phi translation for one of the icmp operands, while keeping the other one, which means that they now refer to values from two different loop iterations.
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've filed #70651 for the JumpThreading issue.
b22917e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XChy @nikic thanks for the prompt reaction!
That's a bit complicated at this point due to all the commits after this one that change the same files. I hope the fix @nikic proposed (#70664) will help mitigate the issue, then there's no need to revert.