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

[JumpThreading] Incorrect elimination of condition #70651

Open
nikic opened this issue Oct 30, 2023 · 2 comments
Open

[JumpThreading] Incorrect elimination of condition #70651

nikic opened this issue Oct 30, 2023 · 2 comments

Comments

@nikic
Copy link
Contributor

nikic commented Oct 30, 2023

The following test case is miscomiled by JumpThreading (https://llvm.godbolt.org/z/o9W6efoTf):

define i64 @test(i64 noundef %v) {
entry:
  %v.nonneg = icmp sgt i64 %v, -1
  br label %for.body

for.body:
  %sum = phi i64 [ 0, %entry ], [ %sum.next, %for.body ]
  %sum.next = add i64 %sum, %v
  %overflow = icmp ult i64 %sum.next, %sum
  %cmp.i5.i = xor i1 %v.nonneg, %overflow
  br i1 %cmp.i5.i, label %for.body, label %cleanup1.loopexit

cleanup1.loopexit:
  %cmp.not.lcssa.ph = phi i64 [ %sum, %for.body ]
  ret i64 %cmp.not.lcssa.ph
}

The %overflow condition is folded to false as part of processBranchOnXOR().

I believe the root cause is that computeValueKnownInPredecessorsImpl() performs phi translation of one icmp operand (the phi node), while leaving the other alone, and then trying to simplify it. This means that we are working on icmp operands from two different loop iterations.

nikic referenced this issue Oct 30, 2023
)

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).
@nikic nikic self-assigned this Oct 30, 2023
@nikic
Copy link
Contributor Author

nikic commented Oct 30, 2023

On the surface, the fix here is as simple as adding a !LoopHeaders.contains(BB) check. However, LoopHeaders is currently only used for heuristics, not correctness, and there is an option to disable it's population.

nikic added a commit that referenced this issue Oct 30, 2023
nikic added a commit to nikic/llvm-project that referenced this issue Oct 30, 2023
When evaluating comparisons in predecessors, phi operands are
translated into the predecessor. If the translation is across a
backedge, this means that the two operands of the icmp will be
from two different loop iterations, resulting in incorrect
simplification.

Fix this by not performing the phi translation for phis in loop
headers.

Note: This is not a complete fix. If the
jump-threading-across-loop-headers option is enabled, the
LoopHeaders variable does not get populated. Additional changes
will be needed to fix that case.

Related to llvm#70651.
@nikic
Copy link
Contributor Author

nikic commented Oct 30, 2023

As this is actively causing issues, let's start with the simple part of the fix: #70664

nikic added a commit that referenced this issue Oct 31, 2023
When evaluating comparisons in predecessors, phi operands are translated
into the predecessor. If the translation is across a backedge, this
means that the two operands of the icmp will be from two different loop
iterations, resulting in incorrect simplification.

Fix this by not performing the phi translation for phis in loop headers.

Note: This is not a complete fix. If the
jump-threading-across-loop-headers option is enabled, the LoopHeaders
variable does not get populated. Additional changes will be needed to
fix that case.

Related to #70651.
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

2 participants