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] SimplifyCFG does not eliminate the UB branch after inlining #98799

Closed
dianqk opened this issue Jul 14, 2024 · 1 comment · Fixed by #98802
Closed

[SimplifyCFG] SimplifyCFG does not eliminate the UB branch after inlining #98799

dianqk opened this issue Jul 14, 2024 · 1 comment · Fixed by #98802

Comments

@dianqk
Copy link
Member

dianqk commented Jul 14, 2024

I tried the following IR:

define internal ptr @bar(ptr %arg, i1 %arg1) {
bb:
  br i1 %arg1, label %bb4, label %bb2

bb2:
  %i = load ptr, ptr %arg, align 8
  %i3 = getelementptr inbounds i8, ptr %i, i64 1
  store ptr %i3, ptr %arg, align 8
  br label %bb4

bb4:
  %i5 = phi ptr [ %i, %bb2 ], [ null, %bb ]
  ret ptr %i5
}

define i32 @foo(ptr %arg, i1 %arg1) {
bb:
  %i = call ptr @bar(ptr %arg, i1 %arg1)
  %i2 = icmp ne ptr %i, null
  call void @llvm.assume(i1 %i2)
  %i3 = load i32, ptr %i, align 4
  ret i32 %i3
}

declare void @llvm.assume(i1)

We can eliminate the branch, but it doesn't. To my surprise, re-executing the output once simplifycfg eliminates this branch: https://llvm.godbolt.org/z/85zKbMdxe.

I've noticed that the user order is different after inlining and reading the text IR. See: #98799 (comment).

The passingValueIsAlwaysUndefined only considers the first user:

static bool passingValueIsAlwaysUndefined(Value *V, Instruction *I, bool PtrValueMayBeModified) {
Constant *C = dyn_cast<Constant>(V);
if (!C)
return false;
if (I->use_empty())
return false;
if (C->isNullValue() || isa<UndefValue>(C)) {
// Only look at the first use, avoid hurting compile time with long uselists
auto *Use = cast<Instruction>(*I->user_begin());

Of course there are many ways to solve this problem, for example:

  1. Handle all users
  2. Choose an instruction that passingValueIsAlwaysUndefined can handle (I'd go with this one first.)
  3. Support assume(%p == null)

But my biggest concern here is whether the user order needs to be consistent. I don't like it when an optimization happens randomly.

@dianqk
Copy link
Member Author

dianqk commented Jul 14, 2024

I think I know why the order of uses is different. Refer to: ef55a1a. All uses are added to the front, so when reading the textual IR, this order is the reverse of the instruction order, and it reverses again during inlining.
I'm not sure how to fix it. Perhaps we need a method to sort them according to the instruction order?

@dianqk dianqk closed this as completed in 8afb643 Jul 16, 2024
yuxuanchen1997 pushed a commit that referenced this issue Jul 25, 2024
…singValueIsAlwaysUndefined` (#98802)

Summary: Fixes #98799.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants