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

In a function with a noundef return value, replace ret undef with unreachable #60717

Open
scottmcm opened this issue Feb 13, 2023 · 6 comments · Fixed by #76656
Open

In a function with a noundef return value, replace ret undef with unreachable #60717

scottmcm opened this issue Feb 13, 2023 · 6 comments · Fixed by #76656

Comments

@scottmcm
Copy link

noundef on the return value means that

If the value representation contains any undefined or poison bits, the behavior is undefined.

so if the function returns undef, that must be unreachable.

Alive2 confirms this is allowed: https://alive2.llvm.org/ce/z/bW2DoM

define i32 @src() noundef {
%start:
  ret i32 undef
}
=>
define i32 @tgt() noundef {
%start:
  assume i1 0
}
Transformation seems to be correct!

But seemingly it doesn't happen today: https://llvm.godbolt.org/z/79a91o59q

@scottmcm
Copy link
Author

@dianqk Will https://reviews.llvm.org/D144319 perhaps fix this issue as well?

@dianqk
Copy link
Member

dianqk commented Jul 23, 2023

@dianqk Will https://reviews.llvm.org/D144319 perhaps fix this issue as well?

Yes, it also solves nonnull related transformations. But this needs to wait until the C++ standard library fixes this UB.
If you need this optimization to be done soon, I can submit a scene that only considers noundef.

@ldionne This UB fix doesn't look like there's been any new progress for about a month now.

@dianqk
Copy link
Member

dianqk commented Aug 6, 2023

https://reviews.llvm.org/D157227
I added a patch to reduce the impact of this UB.

@dianqk
Copy link
Member

dianqk commented Aug 9, 2023

Based on the discussion at D157227, we expect to be back in LLVM 18.

cc @nikic @ldionne

@dianqk
Copy link
Member

dianqk commented Jul 8, 2024

I just realized that the simplest example in the issue is not resolved. lol

@dianqk dianqk reopened this Jul 8, 2024
@ldionne
Copy link
Member

ldionne commented Sep 10, 2024

Just recording that libc++ is out of the way now (and has been for roughly 1 release), since that isn't clear from the thread here.

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.

5 participants