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

[InstructionSimplify] InstructionSimplify error deleted phi #68683

Closed
DianQK opened this issue Oct 10, 2023 · 3 comments · Fixed by #96631
Closed

[InstructionSimplify] InstructionSimplify error deleted phi #68683

DianQK opened this issue Oct 10, 2023 · 3 comments · Fixed by #96631

Comments

@DianQK
Copy link
Member

DianQK commented Oct 10, 2023

I tried this IR:

define i32 @src(i1 %c0, i1 %c1, i32 %v1) {
start:
  br i1 %c0, label %bb0, label %bb1

bb0:
  br label %bb1

bb1:
  %x = phi i32 [ %v1, %bb0 ], [ undef, %start ]
  br i1 %c1, label %bb0, label %bb2

bb2:
  ret i32 %x
}

phi should not be deleted by InstructionSimplify.

alive2: https://alive2.llvm.org/ce/z/LNXeGp

@nikic
Copy link
Contributor

nikic commented Oct 10, 2023

I don't think this is related to CVP? This is InstSimplify behavior.

This poison-propagation hole (and a number of similar ones) has been left intentionally until the "poison on uninit load" semantics come into effect. Though it's possible that the relatively wide use of noundef nowadays allows us to fix this independently. We'd have to evaluate codegen impact.

@DianQK
Copy link
Member Author

DianQK commented Oct 10, 2023

I don't think this is related to CVP? This is InstSimplify behavior.

Yes. The problem is in the

return valueDominatesPHI(CommonValue, PN, Q.DT) ? CommonValue : nullptr;
.

This poison-propagation hole (and a number of similar ones) has been left intentionally until the "poison on uninit load" semantics come into effect. Though it's possible that the relatively wide use of noundef nowadays allows us to fix this independently. We'd have to evaluate codegen impact.

Do you have any suggestions? It looks like this is a rather large change/impact.

@DianQK DianQK changed the title [LVI][CVP] CVP error deleted phi [InstructionSimplify] InstructionSimplify error deleted phi Oct 10, 2023
@DianQK DianQK removed their assignment Oct 10, 2023
@dtcxzyw
Copy link
Member

dtcxzyw commented Feb 16, 2024

It looks like this is a rather large change/impact.

I agree. This has a huge impact on Rust applications due to the lack of noundef on arguments/retvals.

@nikic nikic closed this as completed in dd11636 Nov 7, 2024
Groverkss pushed a commit to iree-org/llvm-project that referenced this issue Nov 15, 2024
…m#96631)

We can only replace phi(X, undef) with X, if X is known not to be
poison. Otherwise, the result may be more poisonous on the undef branch.

Fixes llvm#68683.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants